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

[miv391]

This is my test sprite sheet:

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.

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:

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.

[groud]

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]

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]

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:

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]

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.)

Only changing hframes when vframes > 1 would cause problems:

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]

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

[kleonc]

• 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]

• 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

[YuriSizov]

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

[miv391]

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

Adjustments done.

[YuriSizov]

Thanks!

[YuriSizov]

Cherry-picked for 4.2.2.