Add compatibility with SpriteAtlas #43
Conversation
Search sub-assets now works with Sprite Atlas
|
It makes sense. But we can simplify the codebase; and I believe your code won't fetch Sprites from a folder if that folder is added to SpriteAtlas. To accomplish both of these tasks, we can use an internal Unity function that directly returns the Sprites that are added to a SpriteAtlas. This internal function is already used once in the codebase: Can you make it so that |
|
BTW I now target Unity 2021.3.41f1 so you can remove |
|
Sounds good, let me update the code and let you know. |
|
You are right, my version doesn't handle if the reference is a folder, but using the function you recommended, it works and the code is cleaner. |
yasirkula
left a comment
yasirkula
left a comment
There was a problem hiding this comment.
Thanks for the changes. I've requested a few final changes, please have a look.
| } | ||
|
|
||
| // If is an Atlas we get sprites directly as GetSubAssets return the final texture | ||
| if ( target is UnityEngine.U2D.SpriteAtlas spriteAtlas ) |
There was a problem hiding this comment.
- Can you add a
usingstatement for SpriteAtlas instead? - Since the new code's style is almost the same as the overall coding style of the plugin, I'll ask you to remove the space between
ifand the opening parantheses to match the style exactly.
| subAssets.Add( new SubAsset( packedSprites[i], shouldSearchChildren ?? true ) ); | ||
| } | ||
| } | ||
| return; |
There was a problem hiding this comment.
return makes the code slightly faster but I'd like it to be removed for safety. Imagine SpriteAtlases having sub-assets in the future. That would not work with the return statement.
There was a problem hiding this comment.
Yes, the reason I add the return is because the current add sub assets add the atlas texture and an object with no name to the sub assets list.
| { | ||
| for( int i = 0; i < packedSprites.Length; i++ ) | ||
| { | ||
| if ( currentSubAssets.Add( packedSprites[i] ) ) |
There was a problem hiding this comment.
I've found out that the returned Sprite can be null if it's deleted while the SpriteAtlas hasn't been repacked/rebuilt yet. So a null check is necessary here.
|
All changes done |
| if( packedSprites[i] ) | ||
| { | ||
| if ( currentSubAssets.Add( packedSprites[i] ) ) | ||
| subAssets.Add( new SubAsset( packedSprites[i], shouldSearchChildren ?? true ) ); | ||
| } |
There was a problem hiding this comment.
Thank you for the changes.
For simplicity's sake, I'd like these if conditions to be combined. I can do it myself after the merge but Blame'ing those lines would then show my commit rather than yours. Which one would you prefer?
PS. I used to do if( packedSprites[i] ) a lot but I'm now using if( packedSprites[i] != null ) because I believe it's easier to understand. So if you were to combine these ifs, could you also modify this check?
There was a problem hiding this comment.
Yes, it easier to understand and how this run on the Editor I don't thing is an issue, I used to do a null check without explicit != null because Unity said is faster and safer, but that's apply for runtime, on Editor I don't thing we care about that.
There was a problem hiding this comment.
Actually I would expect the same performance:
There was a problem hiding this comment.
Is an old habit I have all because this old post of Unity: https://unity.com/blog/engine-platform/custom-operator-should-we-keep-it
There was a problem hiding this comment.
That blog post is valid and it talks about the IsNativeObjectAlive function that CompareBaseObjects uses. Both implicit bool operator and explicit == operator call the same function so there can't be a performance difference between them.
There was a problem hiding this comment.
You are right, and by the way thanks for great tool.
2 participants
Search sub-assets now works with Sprite Atlas. This means enabled search sub assets is going to search also references to sprites that are part of the SpriteAtlas