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

Add starting frame to animated_image #9411

Merged
merged 6 commits into from Mar 16, 2020
Merged

Conversation

v-rob
Copy link
Member

@v-rob v-rob commented Feb 18, 2020

This PR does the following to animated_image:

  • Allows the specification of a starting frame (optional parameter).
  • Setting the frame duration to 0 stops the animation.
  • On any event, e.g. a button press, the current frame index is returned using the name parameter.
  • The syntax has been changed: animated_image[<X>,<Y>;<W>,<H>;<name>;<texture name>,<frame count>,<frame duration>;<frame start>]. Notice the two new parameters and the change from a colon to a comma.

This PR is useful because images can be stopped and restarted at any specific point. For instance, a slide show that automatically goes through the images slowly, but buttons can be used to go back to previous images. Or a primitive soundless video with pause, play, and a scrollbar for video position. This can also be used to place specific images from spritesheets in multiple places on the formspec. So, many different applications.

To do

This PR is Ready for Review.

How to test

Use the updated Minimal formspec. The animated images have been changed to numbers to show which frame is currently shown better. The Current Number buttons will send the index of the animated image to the right of them to the chat.

@Df458
Copy link
Contributor

Df458 commented Feb 18, 2020

@kilbith Thoughts re: the syntax?

Personally, I'm indifferent to the syntax changes and the feature seems reasonable enough... It doesn't seem to have many use-cases at the moment, but I assume this will be much more useful when your other proposals start to roll in.

@sorcerykid
Copy link
Contributor

Dumb question, but wouldn't the colon make more sense as a semi-colon to be consistent with other elements where the texture name is always a distinct parameter?

@v-rob
Copy link
Member Author

v-rob commented Feb 18, 2020

Not a dumb question. The answer is probably, but GUIAnimatedImage parses that itself, so it acts as one parameter to GUIFormSpecMenu. I don't think that it's a particularly good design to have parsing in the GUIElement, but that's the way it was, and I won't change it unless someone requests it since I'm working on yet another PR. I made this PR so that we aren't stuck with another element with non-extendable parameters without certain features, but it's rather low priority on my list otherwise.

@sorcerykid
Copy link
Contributor

Ah, okay that sounds like a solid rationale for the comma. I hadn't thought of the internal workings. Thanks for clarifying.

@kilbith
Copy link
Contributor

kilbith commented Feb 18, 2020

I am in favor of commas but <frame start> should be put after a semicolon to be consistent with the other elements like dropdown or table which have the <selected idx> after a semicolon.

@v-rob
Copy link
Member Author

v-rob commented Feb 19, 2020

Ok, I changed the syntax and also moved the parsing code to GUIFormSpecMenu, but don't go testing it yet. I'm going to add an event where, when any event fires, the animated image will tell you what frame is currently shown. This will make it much easier to pause at a specific point.

@v-rob v-rob force-pushed the formspec-anistart branch 8 times, most recently from c088313 to 3feedf8 Compare February 20, 2020 06:09
@v-rob
Copy link
Member Author

v-rob commented Feb 20, 2020

OK, this PR is now Ready for Review. I added the event, and I added some more use cases to the first post. I hope this gets merged before the next Minetest release so that we aren't stuck with the current animated_image syntax and have to do some annoying stuff to add this later.

@kilbith
Copy link
Contributor

kilbith commented Feb 20, 2020

IIRC, if <name> is specified, fields are sent every <frame_duration> time? 👎 This can be abused.

@v-rob
Copy link
Member Author

v-rob commented Feb 21, 2020

No, not every frame duration time. I agree, that would be bad. No; it's only when other events happen. For instance, if you press a button, then the current frame for each image with a name is returned.

@rubenwardy rubenwardy self-requested a review March 1, 2020 14:07
@rubenwardy
Copy link
Member

Would it make sense for the format to be animated_image[<X>,<Y>;<W>,<H>;<name>;<texture name>;<frame count>;<frame duration>;<frame start>], which could make it easier to specify texture modifiers? Is there anywhere where this format is used/similar to?

@rubenwardy
Copy link
Member

Closest appears to be these:

* `[cracko:<tiles>:<frames>:<start>`
* `[verticalframe:<frames>:<start>`

@rubenwardy
Copy link
Member

I guess the benefit of commas is that you can rationalise it at the specification of an entity which is an animated texture, and reuse it in other places

@v-rob
Copy link
Member Author

v-rob commented Mar 2, 2020

I personally have no preference on the syntax. Now that I've moved the parsing code to GUIFormSpecMenu, either one is as easy as the other. I think the idea for commas is that each parameter is necessary to define the image, e.g. a video is meaningless without the number of frames it has.

Edit: Actually, yeah, semicolons might be better. ^[combine uses commas in its syntax, and I don't think you can escape texture modifiers, there being no need to do so. I'll change it to semicolons unless there are strong objections.

@v-rob
Copy link
Member Author

v-rob commented Mar 5, 2020

Two days; I assume there are no objections. I've changed the commas to semicolons

@rubenwardy rubenwardy added this to the 5.2.0 milestone Mar 6, 2020
@paramat paramat removed this from the 5.2.0 milestone Mar 7, 2020
@SmallJoker SmallJoker added the Blocker The issue needs to be addressed before the next release. label Mar 7, 2020
@rubenwardy
Copy link
Member

looks good apart from this, I hope to merge this tomorrow

doc/lua_api.txt Outdated Show resolved Hide resolved
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works

@v-rob
Copy link
Member Author

v-rob commented Mar 10, 2020

Ready for merge at any time.

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@SmallJoker SmallJoker added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 10, 2020
@SmallJoker
Copy link
Member

👍 Changes look good. Thanks.

@SmallJoker SmallJoker removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 14, 2020
@sfan5 sfan5 merged commit 13ad8e2 into minetest:master Mar 16, 2020
aldum pushed a commit to banyamesterseg/minetest that referenced this pull request Apr 16, 2020
@v-rob v-rob deleted the formspec-anistart branch May 12, 2020 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker The issue needs to be addressed before the next release. Feature ✨ PRs that add or enhance a feature Formspec >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants