Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PORT] Removes overlay queuing, saves 6/7 seconds of initialize. Lightly modifies stat tracking macros (+some sprite error fix) #10741

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

[PORT] Removes overlay queuing, saves 6/7 seconds of initialize. Lightly modifies stat tracking macros (+some sprite error fix) #10741

wants to merge 34 commits into from
+134 −284

Conversation

EvilDragonfiend
Copy link
Member

@EvilDragonfiend EvilDragonfiend commented Mar 14, 2024 •
edited
Loading

About The Pull Request

So we have this overlay queuing system right? It's built with the assumption that the "add to overlay list" operation is real expensive, and is thus useful to queue removals or additions.

It turns out that it just isn't, at least during init. In my testing the operation of queuing took LONGER then the actual overlay add/remove did.

That's ignoring the cost of the subsystem's work.

I've also modified part of the stat tracking macro, since it took a good bit of cpu time, and didn't seem to well, do anything. So far as I can tell it always evaluates to 1

This PR does likely bug fix and corrects a weird behaviour
There is some removal of zmimic code but it's fully okay and I checked daedalus overylay subsystem follows this TG version.

Why It's Good For The Game

bug fix

Testing Photographs and Procedure

image

my character is hiding in a pot

image

fire alarm respects alert level

image

cult halo okay

image

mob overlays okay

Changelog

🆑
code: mildly cleaned up flags_1 defines
code: ported a refactor of subsystem overlay from TG. Overlay list is now trustful.
/:cl:

Tsar-Salat
Tsar-Salat requested changes Mar 14, 2024
View reviewed changes
Copy link
Contributor

@Tsar-Salat Tsar-Salat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicks

code/__DEFINES/flags.dm Outdated Show resolved Hide resolved
code/__DEFINES/flags.dm Outdated Show resolved Hide resolved
@EvilDragonfiend
Copy link
Member Author

correcting all icon states are painful

EvilDragonfiend
EvilDragonfiend commented Mar 14, 2024
View reviewed changes
code/controllers/subsystem/overlays.dm
Comment on lines +65 to +70
#ifdef UNIT_TESTS
/atom/var/skip_sprite_error
/obj/item/gun/skip_sprite_error = TRUE
// I hate this but gun sprites are not implemented properly, and unit test blames this
// We'll need to clean up gun sprites someday, but not right now, it's a mess
#endif
Copy link
Member Author

@EvilDragonfiend EvilDragonfiend Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error details can be found in:

Well, it's deleted now because it's too old.
Removing this define will show which wrong icon status we have ingame, especially for guns.

@EvilDragonfiend EvilDragonfiend changed the title [PORT] Removes overlay queuing, saves 6/7 seconds of initialize. Lightly modifies stat tracking macros [PORT] Removes overlay queuing, saves 6/7 seconds of initialize. Lightly modifies stat tracking macros (+some sprite error fix) Mar 14, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 2, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@EvilDragonfiend EvilDragonfiend added Needs Testmerge Test Merged This PR is currently in rotation labels Apr 16, 2024
@EvilDragonfiend EvilDragonfiend removed the Test Merged This PR is currently in rotation label Apr 25, 2024
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@EvilDragonfiend
Copy link
Member Author

fixed now. ready for a review.

@EvilDragonfiend EvilDragonfiend mentioned this pull request Sep 18, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@EvilDragonfiend EvilDragonfiend added Test Merged This PR is currently in rotation and removed Test Merged This PR is currently in rotation labels Nov 19, 2024
PowerfulBacon
PowerfulBacon requested changes Nov 24, 2024
View reviewed changes
Copy link
Member

@PowerfulBacon PowerfulBacon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaks gun overlays

@EvilDragonfiend EvilDragonfiend added the Test Merged This PR is currently in rotation label Dec 10, 2024
@EvilDragonfiend
Copy link
Member Author

echo station CI check failure is fucking nonsense. I will just update the TM.

@lowwy lowwy mentioned this pull request Dec 18, 2024
Closed
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@itsmeow
Copy link
Member

itsmeow commented Dec 31, 2024

image
Still not sure if we want this. I've tested it before locally porting it myself and couldn't find any positive change in init times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tsar-Salat Tsar-Salat Tsar-Salat requested changes

@PowerfulBacon PowerfulBacon PowerfulBacon requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Administration Code Improvement Needs Testmerge Refactor Sprites Test Merged This PR is currently in rotation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EvilDragonfiend @itsmeow @PowerfulBacon @Tsar-Salat