Appearance Weakrefs #1815
Merged
Appearance Weakrefs #1815
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
boring-cyborg
bot
added
Client
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
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. |
Along with related memory optimizations. Saves >1GB of allocations on Paradise init.
| public override MsgGroups MsgGroup => MsgGroups.EntityEvent; | ||
|
|
||
| public MsgNewAppearance() : this(new ImmutableAppearance(MutableAppearance.Default, null)) {} | ||
| public MsgNewAppearance(ImmutableAppearance appearance) => Appearance = appearance; |
Check notice
Code scanning / InspectCode
Convert constructor into primary constructor Note
| public readonly uint? AppearanceId; | ||
| public readonly ScreenLocation ScreenLocation; | ||
|
|
||
| public DMISpriteComponentState(uint? appearanceId, ScreenLocation screenLocation) { |
Check notice
Code scanning / InspectCode
Convert constructor into primary constructor Note
| using OpenDreamRuntime.Objects; | ||
| using OpenDreamRuntime.Procs; | ||
| using OpenDreamRuntime.Procs.DebugAdapter; | ||
| using OpenDreamRuntime.Rendering; |
Check warning
Code scanning / InspectCode
Redundant using directive Warning
wixoaGit
approved these changes
Jan 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fairly substantial rewrite of appearance handling.
There are now two appearance classes:
MutableIconAppearanceandImmutableIconAppearanceMutableIconAppearanceis the class you use for mutable appearance stuff. They are not sent to the client. To send them to the client, you must either useAtomManager.SetAppearance()or if you have a really really good reason useServerAppearanceSystem.AddAppearance().ImmutableIconAppearanceis the class that is sent to the client. It cannot be edited. If you want to edit it, you need to create a mutable copy (which is aMutableIconAppearance). These are held only as weakrefs in theServerAppearanceSystem, and are removed from connected clients when they are garbage collected. This means that if you want to keep it alive you must have a hard ref to it somewhere. Any appearance (mutable or no) which has an overlay/underlay counts as a hard ref for that overlay/underlay.The client has copies of the
ImmutableIconAppearanceand can useToMutable()to edit them locally for animations and such.Appearance ID is now stored on the
ImmutableIconAppearanceand generated by theServerAppearanceSystemKNOWN BUG: /turf animations don't apply, it just sets the final state of the turf appearance. Turf animations can go in another PR.