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

New property TRegion for TCastleImagePersistent allows selecting a cropped region from a UI image set #442

Merged
merged 81 commits into from May 25, 2023

Conversation

Freedomax
Copy link
Contributor

TRegion.1.mp4

Also added a property editor for easy use.
TRegion is actually a TBorder, without re-adding a type (that would also need a separate Enable variable), as TBorder is actually a rectangle, and its default value is easy to keep compatibility with. The newly added property editor (to choose a region from an image) can also be used for TBorder and other purposes, but currently it is only used for TRegion.

@michaliskambi
Copy link
Member

@Freedomax Fantastic! Thank you!

This is a feature I absolutely want, I know many people will be happy using this for UI atlas like https://opengameart.org/content/sci-fi-user-interface-elements , https://opengameart.org/content/rpg-user-interface . The editor for region looks great, I see it snaps to image pixels, super!

I will review in detail later. In particular I'll think about TBorder for this -- on one hand, this works and TBorder is already serializable, so kudos for using this. But maybe we should have just TCastleFloatRectangle or such new serializable component? I'll think about the possible future use-cases of various approaches.

@Freedomax
Copy link
Contributor Author

Use TBorder, setting TRegion using code will be very complicated, so I will design a new TRegion (similar to TRect,but is TPersistent) later. I am still thinking about whether to use the bottom-left corner as the origin or the traditional top-left corner for the coordinate system?

@Freedomax
Copy link
Contributor Author

Freedomax commented Jan 30, 2023

If we use TCastleFloatRectangle, it uses bottom-left corner as origin, if we enlarge the UI atlas size during development, then the coordinates will all become messed up, which is the reason for my hesitation.

@michaliskambi
Copy link
Member

michaliskambi commented Jan 30, 2023

As for basing it on TCastleFloatRectangle (like a TFloatRectangle but persistent):

  1. Indeed we don't need floating-point here, but in other cases having a rectangle with floating-point may be useful. In other engine parts, it often seemed that integers are enough... but eventually we often had to make a change (like for UI coordinates, that are processed as all floats since UI scaling) :)

    So in practice, TFloatRectangle gets more usage than TRectangle, and thus TCastleFloatRectangle may be more useful. So my main argument here is just that "floating-point values, while not necessary here, also do not hurt here, and may make sense for other uses".

  2. As for coordinate system:

    Hm, good point, though I would still say to go with origin in bottom-left. When enlarging the image e.g. in GIMP, one can choose where to place old contents. I know, it's still a bit non-standard that CGE will require to keep old contents in bottom-left (not top-left) to make old coordinates valid... But I think in this case it's important to be consistent with rest of CGE.

    We did choose in general to follow "bottom-left is origin for 2D", with all advantages and disadvantages of this decision :) Advantage: fits better with 3D, Y always goes up, fits right-handed coordinate system, fits OpenGL texture coordinates convention, fits X3D texture coordinates convention. Disadvantage: most other UI systems, 2D image editing software, atlas generation software, maps in Tiled, ... even rows encoded in images follow "Y goes down". So we have conversion code.

    But at least we have one saving grace to anyone stumbled by this: we follow the convention "bottom-left is origin, Y goes up" really 100% consistently in all CGE. So if anyone finds CGE convention weird... OK, it is non-standard compared to various others, but at least it is internally consistent, all CGE API "thinks" in this convention.

    And I think this is strong case to keep region here, like TCastleFloatRectangle and TFloatRectangle, defined from bottom-left origin. Otherwise various API problems will "creep it" (e.g. converting region to TFloatRectangle will be weird).

@Freedomax
Copy link
Contributor Author

Hi, @michaliskambi ,I am not sure how you will implement the TCastleFloatRectangle(wrap TFloatRectangle as a TPersistent instance?), so it may be necessary for you to declare the TCastleFloatRectangle before I update the code.

@michaliskambi
Copy link
Member

Hi, @michaliskambi ,I am not sure how you will implement the TCastleFloatRectangle(wrap TFloatRectangle as a TPersistent instance?), so it may be necessary for you to declare the TCastleFloatRectangle before I update the code.

Makes sense, I'll try to do it today evening. I'll borrow some ideas from TCastleVector4Persistent.

And yes, I think I'll make it used just like TVector4 and TCastleVector4Persistent are. That is:

  • Your code should declare a public property TFloatRectangle, and just use this for all operations.
  • We will add additional TCastleVector4Persistent for the sole purpose of exposing this rectangle values in CGE editor object inspector and serialization, that is: TCastleVector4Persistent can be published.

Note that TFloatRectangle (and so TCastleVector4Persistent too) will have a special state "empty". Which may mean various things in different cases, in this case "empty region" would mean naturally to use the whole image.

This means that from Pascal API, you should mostly just deal with TFloatRectangle.

Freedomax and others added 28 commits March 16, 2023 22:24
… implementation without specifically listing all "XxxPersistent" properties
Despite the comment -- it works perfectly, there's no problem setting it in editor, it seems.
Also small code style adjustments to castleinternalregiondialog.pas
…plicates the code more, and doesn't seem very necessary
Copy link
Member

@michaliskambi michaliskambi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Sorry it took me ages to review + apply.

Done, I committed a few changes, merging!

Some more important changes:

  • I added to this PR a change related to TCastleComponent.ValueIsStreamed, so that CastleInternalRttiUtils doesn't need to have hardcoded all those checks for special XxxPersistent classes.

    I planned this already, and noticed that your changes to CastleInternalRttiUtils related to adding TFloatRectanglePersistent would benefit from it. So diff to CastleInternalRttiUtils is simpler, albeit I needed to change more existing code.

  • I removed RegionEnabled. Testing everywhere just is region rectangle .IsEmpty is simpler, and I didn't observe any drawbacks. And it's also simpler for user to use, editing Region "just works".

A minor UI thing I noticed is that using mouse wheel in the region dialog sometimes shifts the image -- when the mouse cursor is outside of the image, it seems. It's a big surprising when the image is small. It's not a big deal and it doesn't prevent using the dialog for main purpose, so I'll leave this TODO for later.

@michaliskambi michaliskambi merged commit 1afd0b0 into castle-engine:master May 25, 2023
15 checks passed
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.

None yet

2 participants