Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
I was asked to add some smoothing modes for
IconSmoothComponentand was displeased with the system's state enough to refactor it.I believe it's been made much more readable.
Changes:
Removed
SmoothEdgeComponent, which was acting just as a boolean toggle for edge smoothing.Now instead of checking for any sort of toggle, the smooth edge logic is always active. If you don't want to have smoothened edges, just don't add edge layers to the entity sprite.
Removed
SharedRandomIconSmoothSystemIt was completely empty and only used to give client/server versions of this system a common parent. Legacy code artifact?
Various improvements to
IconSmoothSystemIconSmoothComponent.LastPositionwas used to remember the entity's grid position if the entity was unanchored. It was not updated after reanchoring the entity in a different place, which would lead to not updating the surrounding entities after unanchoring it again.IconSmoothSystemused a generation counter to keep track of entities that have already been processed. This was due to using aQueue<EntityUid>to keep track of entities that potentially need their sprite updated. There was very real potential for two entities both standing next to a third to callDirtyNeighbors()and queue said third entity twice.Queue<EntityUid>was replaced withHashSet<EntityUid>. Generational counter and relevant check has been removed.IconSmoothSystemused to passEntityQuery<TComp>objects to methods that use them, instead of keeping them in private fields.EntityQuery<SpriteComponent>,EntityQuery<TransformComponent>andEntityQuery<IconSmoothComponent>have been moved to a private class field. Arguments that were used to pass these to other parts of the system were removed.Resolve<TComp>()/TryComp<TComp>()``Comp<TComp>()over calling the respective methods ofEntityQuery<TComp>, so these could probably be removed. This optimization is only relevant for extremely hot code paths, which get called many times per frame, every frame.IconSmoothSystemdoes not qualify.There was a schizophrenic check at the beginning of
CalculateNewSprite(), which, if failed, short-circuited the method to only update the edge layers. This check was also failed if the entity in question did not haveIconSmoothComponent, or if it has already been processed before (seeQueue<EntityUid>thing above)ifs, and the obsolete parts were removed.Added new smoothing modes :
HorizontalandVerticalThey use the same icon state naming scheme as
CardinalFlagsmode, but only allows connections to the left and right/front and back of processed entity. The connection candidate must also have the same direction.The main purpose of this smoothing mode is for stuff like couches and benches.
There also a considerable amount of refactoring, mainly about deduplicating code or making it less cumbersome.
Everything i did not mention probably falls into this category.
This is still not optimal in my opinion, and some parts could still be improved.
The code related to getting the surrounding entities should be moved up from
CalculateNewSpriteXXXXX()toCalculateNewSprite()This would let us drop duplicated code from
CalculateNewSpriteXXXXX()methods, while also moving edge updating code to be called only inCalculateNewSprite()This will also let us drop the
UpdateEdges()call fromCalculateNewSpriteXXXXX()methods. Currently these methods are responsible forIdeally,
CalculateNewSpriteXXXXX()should only be responsible for the second part.CalculateNewSpriteXXXXX()methods are also used to handle unanchored entities, instead of having a short-circuit with unified logic. The reason for this isCalculateNewSpriteCorners(), which does not have a default icon state per se, but rather 4, applied to different layers.This should probably be moved into a separate method, but I am not sure if it's the best way to do it, Hence, this part is left as-is.
Before calling any
CalculateNewSpriteXXXXX(), there is a weird check for presence of aMapGridComponenton the entity's parent grid. It's weird, because it's only done if the entity is anchored (which implies being parented to a grid.)I left it as-is, since I can not confirm nor deny if it actually does anything.
Media
The upper half of this screenshot shows that things using corner smoothing mode are still working, even if the entities have different rotations.
The lower half shows that smooth edges now also work with rotated entities as well (as shown by the top row of rocks.) Previously, rotating an entity with smooth edges would lead to incorrect edges being hidden.

The bottom row of rocks is for comparison.
This screenshot shows the work of a new

Horizontalsmoothing mode.I re-used
AMEShieldingentity for this, since it (and puddles) are the only entities currently usingCardinalFlagssmoothing.I've drawn red lines to show what connects to what, for clarity.
And here's a bunch of random screenshots of MetaStation, to show that smoothing system still works as intended.
Details
Changelog
Should not affect a regular player in any way.
Hence,
:nocl: