Fix vending_products register continue check #4962
Conversation
|
Is this really needed? If the sprite is regenerated incorrectly it would hit the CRASH() in Insert() at end no? |
As I said, I currently cannot replicate a situation where this is applicable. But as written, the existing code checking If you're worried about this suppressing problems I'm happy to add Merely relying on that super late crash to stop the call is after all the expensive work has already been done is not helpful. Our goal is to prevent superfluous calls to getFlatIcon. |
|
Places like: https://github.com/cmss13-devs/cmss13/pull/4962/files#diff-870b63bb2fc175b1b768c1bb1fb981a02a8d03ba42f2473de87344d048358100L358-L373 further down in the file are correctly continuing. That same code was copied here but was not corrected to be what actually is the key being inserted. |
|
This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself |
About the pull request
This PR cherry picks cmss13-devs/cmss13-pve#68 but removes the extra logging. That PR was attempting to resolve server hitching, but the hitching seems to have gone away with it and a TGS update.
I am not currently able to discern any noticeable differences from this change, however the change looks correct for the continue check to ever work correctly. See Testing Notes below.
Let me know if you do find a change this does improve, or maybe somewhere else we should expect superfluous calls to getFlatIcon.
Explain why it's good for the game
The continue check as written looks like it would never work, whereas now the string that will be used for inserting into the list sprites is the same string we check before creating new icons.
Testing Photographs and Procedure
Testing Notes
Debugging only:
Lobby (LV-624):
One event of registering (master -> init Assets -> vending_products)
55 getFlatIcon calls (.124 total cpu)
Game Start:
No changes
After opening CO vendors and cig vendor:
No changes
Alt+click self:
77 getFlatIcon calls (.353 total cpu)
Alt+click self again:
No changes
After continue change:
Lobby (LV-624):
One event of registering (master -> init Assets -> vending_products)
55 getFlatIcon calls (.117 total cpu)
Game Start:
No changes
After opening CO vendors and cig vendor:
No changes
Alt+click self:
77 getFlatIcon calls (.346 total cpu)
Alt+click self again:
No changes
Debugging only w/ SVN:
Lobby (LV-624):
One event of registering (master -> init Assets -> vending_products)
55 getFlatIcon calls (.124 total cpu)
Game Start:
No changes
After opening CO vendors and cig vendor:
a GET was performed for first CO vendor and first cig vendor opened (never again)
Alt+click self:
77 getFlatIcon calls (.351 total cpu)
A GET is performed for every item listed
Alt+click self again:
A GET is performed for every item listed
After continue change w/ SVN:
Lobby (LV-624):
One event of registering (master -> init Assets -> vending_products)
55 getFlatIcon calls (.120 total cpu)
Game Start:
No changes
After opening CO vendors and cig vendor:
a GET was performed for first CO vendor and first cig vendor opened (never again)
Alt+click self:
77 getFlatIcon calls (.347 total cpu)
A GET is performed for every item listed
Alt+click self again:
A GET is performed for every item listed
Changelog
🆑 Morrow
code: Corrected a check to avoid repeat work in /datum/asset/spritesheet/vending_products/register()
/:cl: