Closed deanljohnson closed 5 years ago
Nice PR, good writeup too š
I did not implement 9-patch data storing in this PR. I think it could easily be added later on so I'd rather get this integrated/approved before we try to add more to this feature.
Agree completely. We can always add more later on, let's keep this one as concise as possible.
No review yet for time reasons, but here's a general roadmap for this PR:
RectAtlas
API and implementation.Might take a bit, given the size of this PR, but we'll get there š
I agree with everything you've mentioned here and that is how I'd like to do it, but doesn't that make this a heavily breaking change since RectAtlas
would not have an API similar to the previous List<Rect>
implementation?
Changing from List<Rect>
to using IList<Rect>
is still technically a breaking change, but it is easily fixed by a breaking change by any users.
I agree with everything you've mentioned here and that is how I'd like to do it, but doesn't that make this a heavily breaking change since
RectAtlas
would not have an API similar to the previousList<Rect>
implementation?Changing from
List<Rect>
to usingIList<Rect>
is still technically a breaking change, but it is easily fixed by a breaking change by any users.
That is a good point, but under the circumstances it doesn't matter: In a commonly used area like Pixmap / Texture atlas lookups, we should avoid breaking changes in both binary and source compatibility, unless we're on a major version step. RectAtlas
replacing the old list of rects will be a binary breaking change either way - so how much it breaks the source doesn't matter going further.
Given that, my approach here would be to find a good design first, and then see how we can integrate it without breaking stuff. Fewer ties to old designs to worry about.
- Ensure backwards compatibility to make this a non-breaking change.
As a preview of what I think could be doable here, we could
List<Rect>
to RectAtlas
, but keep the old API as-is.RectAtlas
API (keep old) to Pixmap and Texture where old API used List<Rect>
.[Obsolete]
.SerializeErrorHandler
in the compatibility plugin that is able to read the old List<Rect>
fields in Pixmap and Texture resources and convert its data to the new RectAtlas
field.[Obsolete]
API and consider renaming the new API to something more fitting or concise now that the old API is out of the way. This last item is off limits for this issue, ignore for now.If this approach can keep full compatibility, we're completely free in how we design RectAtlas
at the cost of having to keep a bit of obsolete API until the next major version step.
@deanljohnson What's the reason for closing this PR?
Summary
Converted
Pixmap
andTexture
to use a newRectAtlas
class for storing atlas information instead ofList<Rect>
. See #697 .Details
RectAtlas
classIList<Rect>
so that it can be used in a similar fashion to the old implementation.Vector2
pivots andstring
tags per indexRectAtlas
.Kown Remaining Issues
List<Rect>
implementation and therefore the Pixmaps have lost their data. This means we probably need to do two things:I did not implement 9-patch data storing in this PR. I think it could easily be added later on so I'd rather get this integrated/approved before we try to add more to this feature.