GUIFormspecMenu: Add animated_image[] element

[ghost]

This is a rebased/updated version of #9032, intended to address the issues that were brought up during review. Since @kilbith wasn't in a position to rebase and complete the original PR, I am adopting in hopes of getting this past the home stretch!

A word of caution. The work on this was split across about a month (with a ~3-week gap) due to some irl issues, so while the main functionality works I can't guarantee that everything was properly handled. Please test/review carefully!

I should also point out that I haven't squashed the original PR's changes yet, I wanted the separation to be clear so that folks could more easily see what I've changed.

How to Test

To test this PR, create a formspec using the new animated_image element and ensure that it renders as expected.

For your convenience, I've attached a minimal test mod:
test_animimage.zip This mod adds the /anim chat command, which displays a fomspec with an animated image inside. I recommend making further changes to the formspec for testing, but this should simplify the process of setting up your test environment.

[Desour]

The new files could use some code comments.

[ghost]

There hasn't been any activity in about a week, but aside from squashing this PR has been all set for review/approval/merge for a while. Any maintainers have the time to look into it this week?

[v-rob]

Could support for other file formats like .jpg be added by splitting the filename at the last period and then concatenating the file number between the name and the extension?

[ghost]

Could support for other file formats like .jpg be added by splitting the filename at the last period and then concatenating the file number between the name and the extension?

Sounds reasonable. I'll look into that tomorrow. Another possibility would be to have some sort of token representing the number in the filename, but I'm not sure it's worth the effort to do. It could potentially make the API clearer though... any opinions on this?

[v-rob]

I don't think having symbols is necessary; it'll just complicate everything. If we leave the numbers as-is, it will be a simple naming convention rule that everyone will consistently follow. But having support for multiple image formats is a little more of important functionality, even if most people use PNG anyway.

[ghost]

I've added support for supplying an extension and rebased. For simplicity, I'm currently assuming .png if the user doesn't supply an extension since I'm fairly certain that anything without an extension will fail to load since the extension seems to matter to the loader (indeed, .jpeg appears to fail loading despite support for .jpg, I believe the bug is unrelated to this feature though).

EDIT: I've now updated my test mod with a second animated_image element loading .jpg textures.

[ClobberXD]

Tested using the test mod provided in the first post, and also tried different combinations of frame duration and frame count. PR works as expected 👍

I thought the duration of jpg images were inconsistent, until I realised that test_1.jpg and test_6.jpg were the same image, lol :)

[SmallJoker]

The code is slim and works as expected. It needs a few stylistic changes, though.

[SmallJoker]

A second core dev opinion would indeed be better than to rely on my comments only. Now that you've changed the PR there are no concept objections I could possibly have. Hopefully that's also acceptable for the others (no answer = passive agreement?).

[kilbith]

I think it's a very bad idea to force using real coordinates implicitly. I do have some complex formspecs which are hard to transit to real coordinates, so they keep using the old system, but I still want to pick up animated images for them.

And it was the same mistake for hypertext.

[ghost]

There was support previously, but @DS-Minetest suggested that I remove it. I could potentially reinstate support, but I'd rather stick to the current policy...

@SmallJoker, I believe you were involved in the approval process for the hypertext PR. What should be done with non-real_coordinates mode on new elements? Same as there, or was that an oversight?

[SmallJoker]

or was that an oversight?

That, and here the same thing happened. Copying the scaling code from the image[] element should be possible and provide "good enough" results. Prior real_coordinates the scaling and positioning is broken anyway, so I wouldn't spend too much effort into making new elements backwards compatible.

[kilbith]

So I have to spend hours to convert my formspec to real coordinates in order to use animated_image or hypertext? How is that hurting to add a one-liner to support the old coordinates system? And how is that clear to someone that these elements works only correctly with real coordinates, since it's implicit?

Are you stupid or what?

[ghost]

Copying the scaling code from the image[] element should be possible and provide "good enough" results.

That's more or less what was there originally. Since we've confirmed that the original change wasn't deliberate, interest has been shown, and the cost is negligible, I've re-instated the feature. @SmallJoker let me know if that's a deal-breaker and I can back out, but since it's so cheap to support I don't really mind keeping it.

Slightly off-topic: @kilbith, I understand that getting things right is important to you and I'm legitimately glad that you're advocating for it... but I feel that your comments sometimes cross the line from spirited debate into creating unnecessary drama. It doesn't bother me personally, but I get the feeling that this sort of thing will be a source of issues sooner or later.

[SmallJoker]

👍 PR looks good again.