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

Entity flips #809

Closed
wants to merge 13 commits into from
Closed

Entity flips #809

wants to merge 13 commits into from

Conversation

Lojemiru
Copy link
Contributor

@Lojemiru Lojemiru commented Jan 11, 2023

NOTE: This PR is not yet ready for a merge. There are still a few cleanup tasks I would like to run, but more importantly some behavior still needs to be solidified and I need to get maintainer attention to figure out what would best align with the current project design direction. As such, I'm opening this for review and discussion while I work out the last few bits of UI polish. This is ready. See most recent reply from me.

Relevant API fork/branch can be found here: https://github.com/Lojemiru/ldtk-haxe-api/tree/EntityFlips

What does this PR add?
The ability to flip Entities on the horizontal and vertical axes within LDtk. This is achieved through the following additions:

  • Two checkboxes added to Entity Definitions to enable/disable flipping on each axis.
  • Two checkboxes added to the Entity Instance Editor to toggle flipping for a given Entity Instance.
  • Hotkeys (x/y) for flipping Entity Instances before placing them in the Entity Tool (along with a visual display of the flip prior to placement and UI dialog boxes copied from tile flips).
  • Three new API fields for two JSON classes:
    • flippableX and flippableY (booleans) in EntityDefJson, which control whether or not instances of a given Entity can be flipped in-editor.
    • f (int) in EntityInstanceJson, which is a 2-bit number used to store flip data identically to the current Tile flip bit format.

Why should this PR be accepted?
Currently, there are two options in LDtk for handling Entities that need to be flipped per instance: Custom Fields (which are neither fast to set en mass nor thoroughly visually represented for this context), and duplicate Entity Definitions with minor differences to display each direction (which does not scale effectively and bloats projects). While these work fine for a variety of contexts, they are inefficient for such a common behavior.

What still needs to be done before this is ready for a final review and merge?

  • UI polish (some of my CSS/HTML doesn't quite align with the rest of the UI - particularly in the EntityInstanceEditor)
  • Pivot handling. This is what I need feedback on. Currently, I do not move the pivot on the EntityInstance at all, as this would likely require extensive changes to EntityInstance handling. I think that most game engines would consider an axis flip to occur based on the pivot, so this likely does need to happen unless the current behavior aligns with LDtk standards. I wanted to check in about this before rolling out the necessary changes due to the time and effort that would be involved in doing so.
  • Reviewing the various tile rendering modes; I expect nineslicing and repeating in particular will have some discrepancies.
  • Replacing my placeholder numbers in the API fork with a realistic release version. This is the absolute lowest priority, as everything else must be finished first.

Images:
image
image

@Lojemiru Lojemiru marked this pull request as draft January 11, 2023 10:54
@Lojemiru
Copy link
Contributor Author

Lojemiru commented Jan 11, 2023

Would like to note that this builds fine when pointed at the modified API, and that the build failures all stem from inconsistencies between this fork and the official API.

@Miepee
Copy link
Contributor

Miepee commented Jan 11, 2023

This addresses the flipping part of #502 , but not the rotation part.

@Lojemiru
Copy link
Contributor Author

UI polish should be done unless @deepnight has any specific complaints. Just had to clean up the input checkboxes for the EntityInstanceEditor. Will be working on the various entity render modes when I next return to this.

@Lojemiru
Copy link
Contributor Author

Alright, I've got all the render modes fixed except for repeat and nineslicing. Repeat could be fixed with some modifications to deepnightLibs, but I don't think it makes sense for tiling textures to flip. Ninesliced textures in Heaps2D don't support flipping at all, to my understanding.

Now we're just down to pivots and positions.

@Lojemiru
Copy link
Contributor Author

I have determined that Entities should flip around their pivots to best align with common engine practices. Remaining TODO items before pivots are in a reviewable state:

  • Resizing UI/UX is currently broken for flipped Entities
  • Placement UI/UX is currently disjointed for flipped Entities
  • Selected graphics draw with an incorrect offset for flipped Entities

@deepnight
Copy link
Owner

Hi,
This seems like quite complicated changes for just flipping the visual render of the entity.
Basically, the goal is:

  • having 2 booleans: flipX/flipY
  • render the texture accordingly

I don't think this should even have any kind of impact on the resizing tool, nor should be affected by pivots whatsoever 🤔
Maybe I missed something?

@Lojemiru
Copy link
Contributor Author

Lojemiru commented Jan 16, 2023

It's mostly complicated because of the pivot handling. This is all really easy if the Entity visually flips but the pivot stays in its original location - however, this is not what the average user expects. They would expect it to flip around the pivot, much like Entities would rotate around the pivot. As such, everything that is based on the Entity Definition's pivot needs to get the adjusted pivot from the Entity Instance instead or the various rendering elements are thrown off.

Here's a couple pictures to show what I mean. This first one is with no pivot adjustment; the Entity visually flips, and nothing else changes. The position of the two objects are (0, 0) and (32, 0).

image

This second one is with pivot adjustment. The Entity visually flips, and also adjusts where its pivot point is both logically and visually. The position of the two objects are (0, 0) and (47, 0).

image

The first scenario is easy to handle on LDtk's side, but becomes far more complicated to handle on most engine implementations. If you've used GameMaker much at all, you could think of this like setting the xscale of an object to -1; it will be horizontally flipped around its origin (pivot in LDtk terms). Instead of just creating the Entity at the listed location, they would need to adjust the placement position of each Entity based on whether or not it has been flipped at the moment of instantiation.

@deepnight
Copy link
Owner

I understand the example, but you're considering the flip as being a visual transformation (ie. in the way Photoshop handles flipping), where as I think, from a dev perspective, you're expecting the data being flipped inside of the entity bounds (eg. a "left slope" becomes a "right slope").

In the second case, the coordinates and the bounds don't change, and you don't have to deal with stuff like "horizontal flip means X coordinate is now offset by 1 cell to the right".

The visual transformation is a different topic, more logical from a GUI perspective, but I don't think it matches the typical use cases you presented in the first post (eg. slopes).

@Lojemiru
Copy link
Contributor Author

Lojemiru commented Jan 16, 2023

For my use case, I am expecting both; if I "flip" an Entity, I expect both its graphics and its hitbox to flip within my game engine. In my experience these are typically both done around the defined origin point for the graphics and hitbox, but it sounds like you've generally worked with the opposite (flipping the hitbox within the original bounds rather than around its origin). If you think flipping purely within bounds is the best use case for LDtk I can work with that, but I don't personally expect most engine integration maintainers to be too happy about it.

@Lojemiru
Copy link
Contributor Author

Lojemiru commented Jan 22, 2023

Hi - any updates on this? I'm okay with reverting my last changes and just implementing Entity flips visually if you think that's the best direction, but I want to make sure we're on the same page as expectations-wise before I finalize the implementation.

@deepnight
Copy link
Owner

Hi, I didn't have time yet to get back working on LDtk. I think we could just split the feature in two parts:

  • basic flipping (UI, key shortcut, and visual flipping of the texture only)
  • pivot based flipping

@Lojemiru
Copy link
Contributor Author

No worries! Just wanted to make sure I hadn't upset you with my tone or anything since it had been about 5 days. Since those would be two separate features, I'll definitely roll this back to just the visual aspect of flips and finish up the PR.

Whenever you do catch the time, it'd be great if you had input on how pivot-based flips should be integrated on the UI/UX and JSON sides. My first instinct would be for it to be a toggle in each EntityDefinition that sets whether they flip within bounds or by their pivot, but I don't want to get ahead of myself since you likely have a better idea of the ideal workflow.

@deepnight
Copy link
Owner

No worry :) I'm just focusing on an important update to my game Tenjutsu and I did put the LDtk update 1.2.6 a little bit further in the future (probably end of January).

The best, from a user perspective, would be your suggestion: having an option to toggle the pivot based vs texture based flipping.

@Lojemiru
Copy link
Contributor Author

Lojemiru commented Jan 24, 2023

Alright, everything should be set. This PR now only addresses visual flips, and disregards the repeat and 9-slices rendering modes due to their design and intended use cases. API PR can be found at deepnight/ldtk-haxe-api#24

image

image

@Lojemiru Lojemiru marked this pull request as ready for review January 24, 2023 07:48
@Lojemiru
Copy link
Contributor Author

I have the pivot-based flipping option just about finished now. Still need to review to make sure it's up to my own standards, but:

  • I'm going to open it as a separate PR predicated upon this one being accepted, as per your request for only one feature per PR.
  • The resulting pivot-flipped Entities are somewhat buggy with the ResizeTool. I have, however, determined that this is due to faults in the ResizeTool's own design relating to EntityInstanceEditor updates and to Entities with pivot points that do not perfectly align with the 9 default options. I plan to remedy these in other PRs, as per your request for only one feature per PR.

@Miepee
Copy link
Contributor

Miepee commented Jul 13, 2023

Has there been any progress for this, reviewing wise? It'd be a really useful feature.

@Miepee
Copy link
Contributor

Miepee commented Oct 13, 2023

@deepnight Is there any update on this? This is a really essential feature, that currently requires me and a few others to run and maintain a fork.
Anything, even a "I currently don't have the time to review this", would be appreciated, as otherwise it gives off an impression that either you don't want these changes, or that it doesn't make sense send PRs to LDtk as the changes will never get merged anyway :(

@deepnight
Copy link
Owner

Hi!
I'm super for giving no proper feedback on this PR. I have plans to implement flipping along with 90° rotation. It's linked to a few behind-the-curtain reworks with some very old components like Resizer and EntityRender, so I can't give a clear estimation on this one right now.
It's definitely planned and quite high on my todo list.

@Lojemiru
Copy link
Contributor Author

Replaced by #982

@Lojemiru Lojemiru closed this Oct 17, 2023
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 this pull request may close these issues.

3 participants