-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: [#285] Use Implicit Tiled layer order to default z index #290
Conversation
Oh yeah I prefer the new constructor for sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Left some suggestions to improve clarity and making property handling consistent for z index values.
Co-authored-by: Kamran Ayub <kamran@hey.com>
Co-authored-by: Kamran Ayub <kamran@hey.com>
Co-authored-by: Kamran Ayub <kamran@hey.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't block on these suggestions but I do think they make the code intent clear.
@@ -78,8 +78,29 @@ export class TiledLayer extends TiledEntity { | |||
*/ | |||
public compression?: TiledCompression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the other class members here aligned correctly?
* Original order of the Tiled layer | ||
*/ | ||
public order!: number; | ||
|
||
/** | ||
* Reference to the raw tiled layer data | ||
*/ | ||
public rawLayer!: RawTiledLayer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
public static parse(layer: RawTiledLayer): TiledLayer { | ||
if (layer.type !== 'tilelayer') throw Error('Cannot parse a non tilelayer type layer'); | ||
const resultLayer = new TiledLayer(); | ||
resultLayer.id = +layer.id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get this is a clever way to coerce but it's not clear what this does at first glance especially for someone not as familiar with hacks in JS.
Might as well use the standard, it's available in all browsers we support and it's clearer.
resultLayer.id = +layer.id; | |
resultLayer.id = Number.parseInt(layer.id); |
Note radix is optional and is 10 by default anyway!
Note also that there was code before that didn't coerce, because this one character can get lost in the noise.
// coerce to integer | ||
return +finalZ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// coerce to integer | |
return +finalZ | |
return Number.parseInt(finalZ); |
😁
Closes #285
This PR implements implicit z-ordering based on the Tiled editor order. Z-index can still be overridden with a custom property if desired. The first layer will start at
-1
z-index (the previous default) and will increment based on the Tiled order.Example of inserted tiles using the implicit z-index, no more custom property!
Small-ish breaking change that moves all the optional properties to an option bag in the constructor
TODOs before merge: