space-station-14
space-station-14 copied to clipboard
space-wizards
Puddle Visuals: ECS/Refactor and fixes
About the PR
This PR removes PuddleVisualizer in favor of PuddleVisualizerSystem and PuddleVisualizerComponent (all Client-side). Generally makes my code for wet floor sparkle effects less janky.
The below screenshot summarizes my design doc with the state of this PR in red. Some things may need to be bikeshedded (such as, should sparkles always convey slipperiness for the average player's benefit?), but the important thing is that I am now the Puddle Visuals Understander, and it's easy for us to bend this system to our will!
Fixes #9049 Fixes #11823 Fixes #6526 (arguably this was fixed by others earlier though)
Screenshots
https://user-images.githubusercontent.com/97707302/196008843-c17608dd-e038-45cb-b801-15d53f1fac26.mp4
Changelog
:cl:
- fix: Fixed tomato and vomit splats losing their color/sprite.
- fix: Fixed wet floor sparkles getting stuck on one animation frame.
- tweak: Wet floor sparkles are now slightly more opaque and stay invisible for less of their animation cycle.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Seeking help on the ubuntu checks failing! This is the same thing that killed my momentum on previous PR months ago 😩
Checks passed but somehow it was just by commenting out the logger warning...
This solution obviously sucks so I'm going to tweak a little more based on #8203 which I just noticed.
All good now and ready for prelim review. To be clear - the logger warning for missing sprites is still intact and not causing problems. For some reason, the logger warning I added for my own system's PuddleVisuals data transfer (e.g. volumeScale, solutionColor) was causing exceptions to be thrown in the tester. This was true whether logger.warning or logger.debug was used.
Since it's for my own system's new data transfer, I'm fine with not having that logger warning. So I just removed it. I suspect this is the same thing causing me problems way back in #6848, because at the time I had bundled my PuddleVisuals data checks within the existing logger warning for missing sprites.
Seeking feedback now, because the only other thing I plan to add for this PR is a check similar to what I've done for isEvaporating but for isSlippery: Evaluate the state within Server puddle system, and transfer to Client puddle system for use in determining wet floor sparkle effects.
Somewhat related to the currently-open #11908 but we change different areas. Should be relatively easy to resolve any merge conflicts between the two.
After polling in the discord, I'm going to revert my latest commit which made sparkles depend on slippery. Will update that later today along with the PR description.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
All good @Ygg01 - I was expecting your changes in #11908 to cause some merge conflicts. Just haven't been able to get into fixing them until now. I'm gonna test this PR some more on local to make sure it's all smoothed out, then this one should be ready for review.
Marked as draft because something broke the "sparkle animation freezes on a single frame" bug again. I had previously fixed this with a sprite.LayerSetAutoAnimated(0, true); and now I intend to go over it again with a fine-toothed comb.
One thing I still wanna do is incorporate @ElectroJr's comment (via Discord) about making the sparkles rsi into a spritespecifier on the component (currently it's just hardcoded in the system which I agree is a bit jank).
Please hold off on merging until I make this small change. And I'd welcome additional review because (for me at least) this was a pretty substantial refactor 😄
oh and now that this one is merged there's another thing I can try to make cleaner (previously had to flip AutoAnimated off and back on again).
I wonder, can this PR help to make puddle more look like a puddle and not like splats - when visually all liquids on floor connected into one big puddle and can change depends on reagent amount ?
I wonder, can this PR help to make puddle more look like a puddle and not like splats - when visually all liquids on floor connected into one big puddle and can change depends on reagent amount ?
Will it make it easier to implement? yes, because not dealing with old code is helpful does this change puddles visually? no, its just code cleanup
Will it make it easier to implement? yes, because not dealing with old code is helpful does this change puddles visually? no, its just code cleanup
For future implementation of course