Made pivot the new location of entities spawned in the world

[mckahz]

The size of the visual area for an entity should only be an indicator to the user about how big it will seem, changing the size should not change where the entity spawned.

This change is how it works in most other engines I've seen deal with it, and I believe it's better to have a defined point which can be seen in the editor to represent the translation of the entity, rather than arbitrarily moving them half way up their visual area.

[Trouv]

Thanks for the contribution! I'm not sure about this. I think there's a lot of situations where you would expect the current behavior. My current project even depends on the current functionality. I would be down to accept it as an option though! That will require creating a new field on the LdtkSettings resource, and I'd prefer the type to be an enum describing the two options rather than a bool

[Trouv]

Maybe the current behavior could be changed to use the Anchor field of a TextureAtlasSprite to provide the pivot functionality instead of changing the transform. We could still make it optional though

[janos-r]

I feel like something should change about the pivot. When using Point values, I find it very difficult to use.
I spent the whole day today looking into the platformer example with Patrol.
It shouldn't be normal that you have to yourself first + Vec2::new(0.5, 1.) (deriving that from the pivot you set in the editor) and than let adjusted_pivot = Vec2::new(0.5 - pivot.x, pivot.y - 0.5);
I still don't fully understand what the 0.5s do. I guess that the translation always has to be in the center of the entity (I didn't know that) and they shift the translation into a spot, so that it ends up looking like in the editor.

Btw, in the fn ldtk_pixel_coords_to_translation_pivoted - it is probably a mistake that the first atribut is called ldtk_coords but it doesn't expect the coords from ldtk (like in the other same-name functions) but real px coords. I would either rename or make the fn take grid_size like the other functions do.

[Trouv]

I feel like something should change about the pivot.

Thanks for commenting, it's always good to know when a suggestion would help others.

I guess that the translation always has to be in the center of the entity

This is true for the default Anchor value of a texture atlas sprite. I think this would all be less confusing if we used Anchor values for the pivot adjustments instead of adjusting the translation. We could make an option for it too.

it is probably a mistake that the first atribut is called ldtk_coords but it doesn't expect the coords from ldtk

I'm not sure this is true, I looked over it again and it looks "correct", unless I'm misunderstanding your suggestion.

[janos-r]

it is probably a mistake that the first atribut is called ldtk_coords but it doesn't expect the coords from ldtk

I'm not sure this is true, I looked over it again and it looks "correct", unless I'm misunderstanding your suggestion.

The other ldtk_coords require to be first multiplied by the grid_size before continuing to treat them as translation coord.
This one in ldtk_pixel_coords_to_translation_pivoted already expects to get previously multiplied coords. Thereby it is not really "ldtk" grid coords but px coords.
Its just a naming thing. The other "ldtk_coord" suggest (like in the other functions) the large grid coords you see in the ldtk editor.

[Trouv]

Thereby it is not really "ldtk" grid coords but px coords.

Ah, I think I see what you're saying. There are other utils functions that expect ldtk pixel coords instead of grid coords, and they also use ldtk_coords as the name of the parameter. But the functions that expect ldtk grid coords instead of pixel coords also call it ldtk_coords. They should all be updated to be more verbose like ldtk_pixel_coords and ldtk_grid_coords. Though, that work belongs in a different PR/issue

[mckahz]

I think there's a lot of situations where you would expect the current behavior. My current project even depends on the current functionality.

Wouldn't you just use a pivot in the center if you needed this behavior? It seems a lot more straight forward than adding more parameters or coupling it to the rendering system.

[Trouv]

Wouldn't you just use a pivot in the center if you needed this behavior?

For many games, yes. But for tile based games, in order to put an entity in the center of a tile, the "center" pivot option won't work. In that case, placing entities on the grid wouldn't put the entity in the center, it would put it on some intersection of the grid. I suppose you could set an offset for the entity layer so it's grid bisects the grids of other layers, but that's not an obvious solution to me..

Anyway, Ive been convinced that this behavior needs to change. I would like to...

1. change the current behavior to use the anchor component rather than changing the transform.
2. make that optional in the settings for users, like yourself, who don't want the pivot to play a role at all in game.

I think these should be separate PRs. We could close this PR or change it so it accomplishes task 2 without doing anchor stuff.

Thoughts? Do you think this solution will work for you?

Again, thanks so much all for bringing this to my attention, I had no idea this was problem for users before this PR.

[mckahz]

change the current behavior to use the anchor component rather than changing the transform.

I don't think it should use the anchor point, since it would be particularly finicky with precise anchors, though I can imagine that being a convenient default to have. How about we add an enum like so

#[derive(Component)]
pub enum TransformImportSetting {
    Pivot,
    Center,
    Anchor,
}

Where pivot is the behavior I want, Center is the behavior it currently has, and anchor is the behavior you're suggesting. This would solve both of your desired changes, no? We could have a function for the app which can register a default behavior for all your imports and adding this component to an LdtkIntGrid bundle could would override the default behavior.

I'm pretty new to both bevy and this library (I can't even get IntGridCell queries to work right haha) but hopefully this is a suitable solution. Also I'm glad I could help! Thanks for the awesome library!

[Trouv]

I don't think it should use the anchor point, since it would be particularly finicky with precise anchors

Could you give an example of where this would be an issue?

Where pivot is the behavior I want, Center is the behavior it currently has, and anchor is the behavior you're suggesting.

Hmm, The naming is tricky here. I think you mean Pivot as in the transform uses the pivot point with no alteration, but in my mind the term Pivot would imply that the translation is "pivoted" to match the editor visual. Maybe Pivot could be the current behavior, and Exact or None could be the behavior you want? I don't know - just spitballing here.

TransformImportSetting

A more specific name would probably be better - maybe LdtkEntityTranslation. I assume all 3 options would still scale up the entity according to its size, so only the translation behavior changes.

I don't think we need to make it a component, a field on the LdtkSettings resource should be fine. If users want to use different options for different entities in the future then we can consider making it a component, but I doubt that's going to be that common of a use case, and at that point users can always update the transforms themselves too. "Settings as resource" is a pretty common pattern for bevy plugins.

[mckahz]

Merry Christmas! Sorry for the late reply I was away visiting family.

I didn't know about the LdtkSettings struct! That makes a lot more sense.

I'd be happy to make it just a setting, and I'd imagine since it's taken as long as it has to raise this issue and the fact that I'd be happy only using the pivot as the translation point I'd imagine that no one needs that level of control. As long as there's a way to get the visual rectangle bounds when hooking into entity creation.

LdtkEntityTranslation makes more sense in every way, so I'm completely on board.

What you said here is accurate

I think you mean Pivot as in the transform uses the pivot point with no alteration, but in my mind the term Pivot would imply that the translation is "pivoted" to match the editor visual.

I'm not exactly sure what you mean by the latter, but regardless we could just be more verbose, I mean hell you're only using it once. And I'm thinking we could either use the Anchor component, or use a custom anchor because that would probably be easy to implement. How about

pub enum LdtkEntityTranslation {
    PivotAsTranslation,
    VisualCenter,
    AnchorOverVisual,
    Custom(Anchor),
}

Just so we can have entities spawn in the center and change how the sprites load in if we need that level of granularity, though I'd be happy to omit the Custom variant.

[Trouv]

Merry Christmas! Sorry for the late reply I was away visiting family.

Merry belated Christmas! Obviously I took a bit of a break too. So, no worries - we're all volunteers here.

I'm not exactly against the possibility of a Custom option, or a VisualCenter option for maintaining the current functionality, at least not in terms of the utility they provide. However, even though this is such a small part of the plugin, having this many variants for the option does increase the cost of maintenance. It's more variants to update in the event that any translation/anchor-related breaking changes are introduced to bevy, and creates more combinations to worry about testing before new releases. So, I'm somewhat hesitant about adding more options to things even if they're easy to implement unless it's clear to me that enough users would use those options. So, I'm not totally convinced that we need the custom option, and I would like to hear an example where using the VisualCenter option would preferable to just using AnchorOverVisual.

Also, naming things is hard... Now that I think about it, not all of these options are totally related to the literal "translation". After all, PivotAsTranslation, AnchorOverVisual and Custom would all use the same translation, it's only the anchor that changes. So, I'll add another iteration to our naming suggestions here:

pub enum VisualPlacementStrategy {
    Anchor,
    Translate,
}

pub enum LdtkEntityPlacement {
    MatchVisual(VisualPlacementStrategy),
    PlainPivotPoint,
}

Or, if we don't have a VisualCenter/MatchVisual(Translate) option, we could simplify:

pub enum LdtkEntityAnchor {
    FromPivot,
    None,
}

I'm trying to make a distinction between "pivot" and "pivot point", where pivot refers to the pivot field of an ldtk entity that defines where the entity's visual is in relation to its pivot point, and pivot point refers to the actual pixel coordinates of the entity in a level. Thoughts?

[mckahz]

I'm trying to make a distinction between "pivot" and "pivot point", where pivot refers to the pivot field of an ldtk entity that defines where the entity's visual is in relation to its pivot point, and pivot point refers to the actual pixel coordinates of the entity in a level.

I like that there's you're trying to make a distinction here, but I doubt any explanation of this would reach the users, and I think we can make it clearer.

I agree that adding all these options is probably more work than it's worth (and I can't really think of any examples, it just feels like it's coupling things that feel like they shouldn't be coupled), and quite frankly the only 2 options we collectively care about are using the pivot as a translation point and using the visual area's center as the translation point.

Without the other options, we don't have to worry about how the sprite will now render and the name "translation" makes a lot more sense.

pub enum LdtkEntityTranslation {
    VisualCenter,
    Pivot,
}

I'd be fine without the extended functionality, and I think this is very clear / easy to use. But yeah, it's a small part of the library and it feels like we're thinking too hard about what we name them, as long as there's something in the docs in an easy to find location pointing to this api then I think that works just fine.

[Trouv]

Yeah we're definitely getting a little too hung up on the naming. I think we should just focus on incremental improvements. This LdtkEntityTranslation option, which doesn't do anything with bevy's Anchor, would be an improvement over what the plugin does now - and would be a welcome contribution. We can add Anchor stuff later if it's desired.

I think that would be a good way to wrap up this PR - add that simple 2-variant LdtkEntityTranslation option. Although I would suggest naming the Pivot variant PivotPoint still.