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

Fix invalid frame index when Sprite2D's hframes or vframes change #85317

Merged

Conversation

miv391
Copy link
Contributor

@miv391 miv391 commented Nov 24, 2023

This is my test sprite sheet:

kuva

In master this code causes an error as expected:

	sprite_using_sheet.hframes = 2
	sprite_using_sheet.vframes = 2

	sprite_using_sheet.frame = 4 
# --> main.gd:15 @ test2(): Index p_frame = 4 is out of bounds (vframes * hframes = 4).

But this is fine:

	sprite_using_sheet.hframes = 2
	sprite_using_sheet.vframes = 2 # now there are frames 0-3
	
	sprite_using_sheet.frame = 3
	
	sprite_using_sheet.hframes = 1 # now there are frames 0-1, but frame is still 3 

Now the sprite looks very odd. The size and shape match to the spritesheet's current frame size and shape, but the content is garbage.

kuva

If the same is done in editor's property Inspector, the result is more or less the same. Editor however changes the frame property to 1, but still the sprite looks wrong.

With this PR the sprite looks like this when the code above is run:

kuva

How vframes and hframes changes can affect frame

What cases this PR handles and how. The goal is to make sure that the frame value keeps valid (must be smaller than hframes * vframes) and if possible, to keep frame referencing the same row and column in the texture sheet even if vframes or hframes changes.

vframes changes

         (a)       (b)

OOO  ->  OOO  or  OOO
OOO      OOO      OOO
OOO      OOO
         OOO

(a) frame's validity or value cannot change.
(b) frame can become invalid, if its row is dropped. frame's value cannot change.

hframes changes

Special case where vframes is 1:

          (c)          (d)

OOOO  ->  OOOOOOO  or  OO

(c) frame's validity or value cannot change.
(d) frame can become invalid, if its column is dropped. frame's value cannot change.

When vframes > 1:

          (e)          (f)

OOOO  ->  OOOOOOO  or  OO
OOOO      OOOOOOO      OO 
OOOO      OOOOOOO      OO

(e) frame cannot become invalid, but its value can change.
(f) frame can become invalid if its column is dropped, also frame's value can change.

  • Cases (a) and (c) require no handling.
  • Cases (b) and (d) are handled simply by checking if frame <= hframes * vframes. If not, frame is reset to 0.
  • Only the cases (e) and (f) need more special handling. If the frame was located in a column that was dropped, reset frame to 0. Otherwise calculate original row and column and then calculate new frame value using new hframes values. Finally the if frame <= hframes * vframes check is done.

@miv391 miv391 requested a review from a team as a code owner November 24, 2023 17:01
@Calinou Calinou added bug topic:rendering topic:2d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 24, 2023
@Calinou Calinou added this to the 4.3 milestone Nov 24, 2023
@groud
Copy link
Member

groud commented Nov 24, 2023

I think the fix is needed, but I would probably set it to the highest value instead.
Something like this:
frame = MIN(frame, (vframes * hframes) - 1)

@miv391
Copy link
Contributor Author

miv391 commented Nov 24, 2023

I think the fix is needed, but I would probably set it to the highest value instead.

I though about that too. But I'm guesssing that in most use cases it just doesn't matter which new valid value is chosen, it is not exactly right for the user anyway. Let's say the user has a sprite sheet of 4 rows x 6 columns. Sprite currently uses the last frame (23). Then user decides that the last row is no more needed and changes the sheet to 3 rows x 6 columns. Most likely the new last frame (17) isn't any better choice for the sprite than the first frame. In my opinion the first frame is just a more intuitive default value when such a value is needed. My other guess is that most often this bug happens when user is editing sprites and sprite sheets in editor so the user immediately sees what's happening. Frame just resetting to 0 is maybe less startling than frame jumping from 23 to 17.

@akien-mga
Copy link
Member

akien-mga commented Dec 12, 2023

Makes sense to me to handle this case indeed.

Regarding what frame should be set to, it's a difficult question. I agree with @miv391's example that in some circumstances just clamping to the last frame may not yield any useful/predictable result.

But that made me realize that this is a problem any time hframes or vframes is changed, even if the change doesn't make frame go out of bounds. Changing the h and v frame count will change the meaning of frame anyway.

For example with the spreadsheet in the OP:

kuva

If frame is 1 (blue "2") and hframes is reduced to 1, then if frame stays 1 (so only first column, red "1" and yellow "3"), the Sprite2D will now show the yellow "3". Which is completely arbitrary.

So maybe we should always reset frame to 0 when either hframes or vframes is changed? And this should be documented in the description of hframes and vframes.

Alternatively, we could try to calculate the would be frame number for a given (x, y) frame before and after the hframes/vframes change, to change frame so that it still points at the same rect - provided it's still in the region, otherwise it would indeed have to be set to 0 or clamped.

@miv391
Copy link
Contributor Author

miv391 commented Dec 12, 2023

So maybe we should always reset frame to 0 when either hframes or vframes is changed? And this should be documented in the description of hframes and vframes.

I think always resetting frame to 0 would be overkill. There are afterall cases where changes do not affect frame. For example, you could just add more rows to the texture. vframes changes, but frame still references the same rect as before. Or you have all your frames in a long row and you add one more frame. Now hframes changes, but frame still references the same rect as before. In the pictures below I'm adding new rows/columns, but removing rows/columns would work in the same way, frame could become invalid, but if it stays valid, its rect stays the same.

(Red box represents the direction to where the user is expanding the texture.)

kuva

Only changing hframes when vframes > 1 would cause problems:

kuva

Alternatively, we could try to calculate the would be frame number for a given (x, y) frame before and after the hframes/vframes change, to change frame so that it still points at the same rect - provided it's still in the region, otherwise it would indeed have to be set to 0 or clamped.

This would likely be the most elegant solution.

@miv391 miv391 force-pushed the fix-breaking-frame-index-in-sprites branch from 0201ebe to d506d84 Compare December 12, 2023 16:48
@miv391 miv391 requested a review from a team as a code owner December 12, 2023 16:48
@miv391
Copy link
Contributor Author

miv391 commented Dec 12, 2023

I changed how set_vframes works and also updated the PR's description.

@kleonc
Copy link
Member

kleonc commented Dec 12, 2023

  • Sprite3D's logic needs to be kept in sync.
  • Guards for early return could be added to the setters, like if (vframes == p_amount) { return; } (including in set_frame).

@miv391 miv391 force-pushed the fix-breaking-frame-index-in-sprites branch from d506d84 to 000f2f1 Compare December 12, 2023 18:50
@miv391 miv391 requested a review from a team as a code owner December 12, 2023 18:50
@miv391
Copy link
Contributor Author

miv391 commented Dec 12, 2023

  • Sprite3D's logic needs to be kept in sync.

I added same logic to Sprite3D.

  • Guards for early return could be added to the setters, like if (vframes == p_amount) { return; } (including in set_frame).

A separate PR exists for adding property change guards: #85311

@miv391 miv391 force-pushed the fix-breaking-frame-index-in-sprites branch from 000f2f1 to 45b7579 Compare December 12, 2023 19:01
@YuriSizov YuriSizov requested a review from groud December 15, 2023 15:48
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Seems good to me, but needs adjustments to the docs and comments.

doc/classes/Sprite2D.xml Outdated Show resolved Hide resolved
doc/classes/Sprite2D.xml Outdated Show resolved Hide resolved
doc/classes/Sprite2D.xml Outdated Show resolved Hide resolved
doc/classes/Sprite3D.xml Outdated Show resolved Hide resolved
doc/classes/Sprite3D.xml Outdated Show resolved Hide resolved
doc/classes/Sprite3D.xml Outdated Show resolved Hide resolved
scene/2d/sprite_2d.cpp Outdated Show resolved Hide resolved
scene/2d/sprite_2d.cpp Outdated Show resolved Hide resolved
scene/3d/sprite_3d.cpp Outdated Show resolved Hide resolved
@miv391 miv391 force-pushed the fix-breaking-frame-index-in-sprites branch from 45b7579 to 484c5b5 Compare December 15, 2023 18:32
@miv391
Copy link
Contributor Author

miv391 commented Dec 15, 2023

Seems good to me, but needs adjustments to the docs and comments.

Adjustments done.

@YuriSizov YuriSizov merged commit 1e86ce0 into godotengine:master Dec 16, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@miv391 miv391 deleted the fix-breaking-frame-index-in-sprites branch December 16, 2023 18:38
@YuriSizov YuriSizov changed the title Fix invalid frame index when Sprite2D's hframes or vframes has been changed Fix invalid frame index when Sprite2D's hframes or vframes change Dec 21, 2023
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants