Respect pivot in tile layers #162
Conversation
|
Finally starting to look at this. It's kinda scary changing stuff like this. Here's an example why - check out the platformer example, the tops of the ladders don't seem to be acting as ladders. Not sure why yet |
|
I've found the bug, it was this line: max: IVec2::new(layer_instance.c_wid, layer_instance.c_hei),It should have been max: IVec2::new(layer_instance.c_wid - 1, layer_instance.c_hei - 1),During debugging I've tried out several things and am now wondering if it would be better to have one entity that corresponds to the layer (like before, but without the I am also wondering if we should add spatial bundles only to tile entities created from the IntGridCell, because all other entities should probably only be used for rendering. |
This makes sense to me - though I'm not sure off the top of my head if If that hierarchy is required, I think that's fine. That would be a breaking change for many users, but it sounds like a logical hierarchy to me and it's not like we're a stable library.
|
| IVec2::new(a.px.x.div_euclid(grid_size), a.px.y.div_euclid(grid_size)) | ||
| } | ||
|
|
||
| struct SubLayer { |
There was a problem hiding this comment.
Could you add some docs for this type? Mostly for maintenance purposes - what its purpose is, why it's designed like this, etc.
I'm also tempted to ask you to move it to a new private sub_layer module - but I will likely refactor a lot of this level-spawning code later so I'll say that's optional
| } | ||
|
|
||
| #[derive(Default)] | ||
| struct SubLayers { |
There was a problem hiding this comment.
Some basic docs here would be nice too
| } | ||
|
|
||
| #[derive(Default)] | ||
| struct SubLayers { |
There was a problem hiding this comment.
NIT: this is much more than just a collection of sublayers, and I think the name could reflect that. Maybe like TileLayerBuilder?
| }) | ||
| } | ||
|
|
||
| fn unwrap(self) -> Vec<SubLayer> { |
There was a problem hiding this comment.
NIT: Could we call this something other than unwrap? That term in my mind is associated with unsafe error handling, or mutexes. Maybe finish()?
| false | ||
| } | ||
|
|
||
| fn insert(&mut self, tile: TileInstance, grid_size: i32) { |
There was a problem hiding this comment.
It'd be nice to get some tests for these new methods you've added. This one especially is not trivial.
| overflow.push(tile); | ||
| } else { | ||
| layer.push(tile); | ||
| layers.insert(tile, grid_size); |
There was a problem hiding this comment.
I think it would be cool if the insert method guaranteed that the no intersections occurred itself. That way, there would be no human error in the case that some future dev tries to run insert before checking intersects first.
It's already looping through all the layers to check their offsets, so it wouldn't be very costly to also check the intersection within that loop, rather than out here. Then instead of insert returning nothing, maybe it could return something like:
enum SubLayersInsert {
Success,
Overflow(TileInstance),
}If you went ahead with moving the sub layer logic to another module, then you could help regulate these type guarantees further by keeping the internal field private and only publicize insert and finish to the rest of the crate. However, like I said, I will be doing some refactoring soon so this is definitely optional.
| .collect::<Vec<_>>() | ||
| }) | ||
| .enumerate() | ||
| if layer_instance.layer_instance_type == Type::IntGrid |
There was a problem hiding this comment.
After some thought, I have to say that I'm not entirely convinced that the intgrid sub-layer should be completely separate from the autotile sub-layers that render it. Much of the point of using bevy_ecs_tilemap is that each tile is an entity, and with ECS we can easily tack-on the intgrid values to these tile entities. I think it's more in the spirit of ECS to not provide multiple purposes to these entities by making them siblings, but instead to just give the same entity more components. Though I do admit it has its limitations w/ this PR in particular, and it definitely complicates things. Thoughts?
This should fix #152.
Tiles which are not aligned to the grid are put into own layers, one layer for each encountered alignment offset.
They also only span the part of the grid in which tiles are found.
Another separate layer is added that contains the entities for the int grid cells.