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

Property class renamed to type again since Tiled v1.10.0 #446

Closed
JumpLink opened this issue Jul 5, 2023 · 2 comments · Fixed by #447
Closed

Property class renamed to type again since Tiled v1.10.0 #446

JumpLink opened this issue Jul 5, 2023 · 2 comments · Fixed by #447

Comments

@JumpLink
Copy link
Contributor

JumpLink commented Jul 5, 2023

This change of #384 was reverted since Tiled v1.10.0, see https://www.mapeditor.org/2023/03/10/tiled-1-10-released.html

It's still called Class in the GUI but in the file format it's type again, so I think we can name it Class but we should handle the type property or to remain backward compatible both properties type and class , something like this:

   public getObjectsByClass(type: string): TiledObject[] {
      return this.objects.filter(o => o.class?.toLocaleLowerCase() === type.toLocaleLowerCase() || o.type?.toLocaleLowerCase() === type.toLocaleLowerCase());
   }
@eonarheim
Copy link
Member

@JumpLink Thanks for the issue!

I think this is a great way to handle this! Should we also add another method getObjectsByType()?

@JumpLink
Copy link
Contributor Author

JumpLink commented Jul 5, 2023

@eonarheim This method getObjectsByType() still exists in excalibur-tiled and is marked as deprecated since Tiled v1.9.0. Tiled originally wanted to change the property from type to class to be consistent, but there were too many complaints about this. So I think we can also call this class and not need a method called getObjectsByType even if the property is actually called type internally again for backward compatibility. I don't have any particular opinion about it and I don't know if the property type is needed for anything else either, but I suspect that this is not the case, otherwise it would become even more confusing.

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

Successfully merging a pull request may close this issue.

2 participants