Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it easier and more efficient to access a property by name #54

Closed
kurtome opened this issue Sep 13, 2022 · 5 comments · Fixed by #55
Closed

Make it easier and more efficient to access a property by name #54

kurtome opened this issue Sep 13, 2022 · 5 comments · Fixed by #55
Labels
enhancement New feature or request

Comments

@kurtome
Copy link
Collaborator

kurtome commented Sep 13, 2022

What could be improved

Properties on tiles and objects are unique, but are returned as List<Property>, which makes it difficult to access a property by name and requires iterating through the entire list each time.

So, I'm suggesting changing tile.properties and object.properties (and anywhere else applicable) to Map<String, Property>, for better access.

Why should this be improved

The Tiled editor doesn't allow multiple properties with the same name, and accessing the properties by name would better match the Tiled editor. In addition, if the properties are keyed by name it will be more efficient.

Any risks?

This would be a breaking change.

@kurtome kurtome added the enhancement New feature or request label Sep 13, 2022
@spydon
Copy link
Member

spydon commented Sep 13, 2022

Sounds very sensible, and since tiled is pre-v1, it's definitely worth the breaking change.
It'll break for flame_tiled users too ofc, but I think it is worth doing these quality of life fixes. flame_tiled wasn't supposed to be >v1, we accidentally released that as that when we released Flame v1.
Should I assign you to the issue?

@kurtome
Copy link
Collaborator Author

kurtome commented Sep 13, 2022

Sounds very sensible, and since tiled is pre-v1, it's definitely worth the breaking change.

It'll break for flame_tiled users too ofc, but I think it is worth doing these quality of life fixes. flame_tiled wasn't supposed to be >v1, we accidentally released that as that when we released Flame v1.

Should I assign you to the issue?

Yup, I can make this change

@jtmcdole
Copy link
Collaborator

Can we avoid breaking by using:

  1. a new sensible name
  2. @deprecated('use sensible name') the properties today?

@kurtome
Copy link
Collaborator Author

kurtome commented Sep 14, 2022

Can we avoid breaking by using:

  1. a new sensible name

  2. @deprecated('use sensible name') the properties today?

We could! But if we don't really care about backwards compatibility, might as well keep the simple name "properties"?

@spydon
Copy link
Member

spydon commented Sep 14, 2022

Can we avoid breaking by using:

  1. a new sensible name

  2. @deprecated('use sensible name') the properties today?

We could! But if we don't really care about backwards compatibility, might as well keep the simple name "properties"?

Is there something called properties in tiled? If that is the case I'd definitely keep it. And as you said, we're don't care that much about breaking changes here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants