godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.17k stars 98 forks source link

Add support for loading (and/or importing) animated images (GIF, APNG) #1433

Closed Xrayez closed 3 years ago

Xrayez commented 4 years ago

Describe the project you are working on:

Goost Godot Engine Extension.

Related PR: goostengine/goost#8.

Describe the problem or limitation you are having in your project:

Godot doesn't currently support loading animated images (such as GIF), and there are no core data structures to facilitate this. Currently, we are limited to a single Image class which acts as a container for static images and provides basic image processing.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:

I propose that we add some basic (but still useful) support for loading (and possibly importing) animated images. For Godot, this would be mainly useful for the AssetLib godotengine/godot#31683, where people may link animated images into descriptions/relevant sections, for instance see my plugin https://github.com/Xrayez/godot-editor-icons-previewer with some GIF showcasing the plugin functionality.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

GIF

Minimal core functionality is required to make this happen. For introduction, see godotengine/godot#31831. Since the PR hasn't received much review from the core developers throughout a year (by the way, tomorrow will be exactly one year since the PR was opened!), it seems like that PR got abandoned (CC @Daw11, feel free to take over and/or discuss this).

I'm not sure the reasons why it wasn't accepted for 3.2, but I suspect that the PR provided too much functionality which was considered a bloat for Godot. So, I'm taking a different approach and propose an alternative solution which should be more self-contained and easier to maintain, see my PR at goostengine/goost#8. Obviously, I had to strip out (most) non-essential features.

The most core data structure which was originally proposed is AnimatedImage, which basically acts as a container for an array of Images with a custom delay for each. Different loaders could then use this class as a proxy for animated image importers.

I decided to rename AnimatedImage class to ImageFrames. I recall Akien wasn't particularly happy with Image name for the existing class and once asked whether it makes sense to rename it to ImageData so people are less likely to use that as Textures, see #godotengine-devel IRC logs:

Akien> reduz: Should we rename Image to ImageData or similar? We often get users confused and trying to use Image to load a PNG texture (where they should use StreamTexture or ImageTexture)
This being prompted by #41106 - but the UX problem around handling Image, ImageTexture and StreamTexture might run a bit deeper than just naming.

I believe it wouldn't be an improvement because then we'd have to rename other Image-related methods such as create_from_image or get_data. But renaming AnimatedImage is indeed a good decision in my eyes in comparison, to avoid further confusion with AnimatedTexture class, so I'll refer to it as ImageFrames from now on.

Another reason for having ImageFrames is because users could benefit from loading animated images via GDScript, with an API similar to Image:

var frames = ImageFrames.new()
frames.load("path/to/your.gif")` # Or apng, not supported yet.
frames.load_gif_from_buffer(bytes)` # Would be certainly useful for HTTP requests and AssetLib.
frames.load_apng_from_buffer(bytes)` # Doesn't exist yet.

Personally loading GIF images dynamically is the main motivation for me to work on this in fact, I'm not particularly interested in importers, but importing GIFs as AnimatedTextures would be certainly useful, with the abundance of GIFs out there.

I kinda "had" to remove ImageFrames::to_animated_texture and ImageFrames::to_sprite_frames because they can be implemented via script, this a compromise to increase the chances of this feature to be eventually implemented in Godot without bloat, I could certainly re-add these methods in Goost.

On that matter, I'd also like to propose to add AnimatedTexture.create_from_image_frames method, if this ends up implemented in Godot. This would be in alignment to ImageTexture.create_from_image method.

To summarize, we have Image for static images, so makes sense to add ImageFrames class for animated images (again, the purpose of both these classes currently is exactly image data storage, with some processing involved). Of course it would be up to the user to decide how these frames could be converted to AnimatedTexture or SpriteFrames, or AnimationPlayer with frames etc. I think importers could be completely implemented by the user, but I haven't tried this, the PR I linked only supports importing as AnimatedTexture (no SpriteFrames etc).

So, I'd like to come up with some common API at the very least with this proposal. If goostengine/goost#8 deemed acceptable for Godot, then I can start working on porting this to master branch. Not implementing importers at all is also acceptable for me.

If this enhancement will not be used often, can it be worked around with a few lines of script?:

GIF is a known format for which there's an existing C library which works reliably for loading GIF images, cannot be possibly implemented via script.

Is there a reason why this should be core and not an add-on in the asset library?:

This can definitely be implemented as a C++ module as seen in goostengine/goost#8. If this proposal is not accepted, the PR is going to be merged eventually in Goost, hopefully that's alright considering this feature attracted a lot of 👍 and both projects are MIT licensed, I'll properly add @Daw11 as a co-author and even invite him as a collaborator, if that's the case.

groud commented 4 years ago

I don't really see the point. Can't we simply allow gifs to be imported to the existing resource of your choice ? I mean, for 2D images you can choose how you want to import the resource (bitmap, cubemap, texture2D, etc...), so we could simply allow the image to be imported as AnimatedTexture or SpriteFrames resources.

Xrayez commented 4 years ago

I always felt like Godot is limited to providing consistency in how loaders vs importers are implemented. Currently, a large chunk of importing functionality in Godot is implemented in C++ and currently inaccessible to scripting. This is important if we consider a proper modding support, it doesn't have to be limited to import process.

The main takeaway in the proposed ImageFrames class is that it does provide useful API to facilitate this at "run-time" as well as "import-time", especially for scripting. Again, read my points regarding AssetLib, for instance. Loading images in C++ is implemented via ImageLoader class which is dedicated for that purpose. Everything else builds upon this logic, including importers. That said, it does simplify implementing various kinds of importers per file format, without bloating Godot binary size by actually reusing common loader classes, in exchange to quote unquote "bloating" API with ClassDB::register_class<ImageFrames>.

By the way, the Image resource itself can be imported currently in Godot. But I'm not even proposing importing ImageFrames, in spite of this. Many texture importers rely on the Image class as a proxy for importing images to Image itself, Texture, BitMap etc. So, makes perfect sense to me to have this kind of consistency.

@groud following your logic, do you have some kind of existing notion that Image methods such as load_png_from_buffer should be removed in Godot just because we already have a way to import them?

Xrayez commented 4 years ago

Can't we simply allow gifs to be imported to the existing resource of your choice ?

To clarify, this is already the case, but it goes through building up ImageFrames first which aims to be agnostic to the imported resource type, as well as providing animated image loader function implementation as well. Again, this is already the case for Image, where different image loader functions are assigned directly in the class as well.

Xrayez commented 4 years ago

so we could simply allow the image to be imported as AnimatedTexture or SpriteFrames resources.

Unfortunately, none of the existing AnimatedTexture nor SpriteFrames support all of the needed functionality of animated images, as described by the original author in godotengine/godot#31831:

I'm not sure of which data structure is the best for this kind of files. AnimatedTexture is simple and it allows to set a custom delay for each frame of the animation. The problem is that it has a limit of 256 frames.

SpriteFrames has the opposite problem, it doesn't have a limit to the number of frames but it can't set a specific delay for each frame.

so this is another rationale for implementing a dedicated ImageFrames class.

groud commented 4 years ago

So if I understand your point correctly, your issue is only to be able to load animated images like gifs at runtime ? Considering you can already have PCK files for modding, the use case seems quite limited no ? Or maybe I misunderstood something.

If the goal is to have something as flexible as Image for animated images, I don't see this happening. The use case looks unusual, and can be easily workarounded by using multiple normal images in an AnimatedSprite.

Unfortunately, non of the existing AnimatedTexture nor SpriteFrames support all of the needed functionality of animated images, as described by the original author in godotengine/godot#31831:

Well, then we could simply modify those classes to support the missing features, which is a lot simpler fix.

Xrayez commented 4 years ago

So if I understand your point correctly, your issue is only to be able to load animated images like gifs at runtime ? Considering you can already have PCK files for modding, the use case seems quite limited no ? Or maybe I misunderstood something.

That's more like comparing apples to oranges. My use case is more like for user-generated content. A regular player won't be able to create PCK files even if you provide them the tools to do this. Also, you'd have to consider #1212 because I don't believe the interface for this to be usable in practice, or it's just a lack of documentation, I don't know.

The use case is already quite limited even for loading static images, mind you, but there's still support for that in Godot, which is great and perhaps I just got lucky enough to have those image loading methods in the first place.

If the goal is to have something as flexible as Image for animated images, I don't see this happening. The use case looks unusual, and can be easily workarounded by using multiple normal images in an AnimatedSprite.

Don't jump to conclusions that fast, I definitely not proposing recreating image processing in ImageFrames functionality just like in Image, that would be overkill. Again, the purpose of this class is more like container type rather than something which can be used instantly by the engine (for instance, you have to create ImageTexture to display/render an Image currently).

Well, then we could simply modify those classes to support the missing features, which is a lot simpler fix.

Well, despite the limitations, I'm not sure whether it's reasonable to feature-creep those classes, I think they have their own purpose and they do the best thing they were initially created for.


@groud you haven't commented on AssetLib usage and whether you think that existing methods such as Image.load_png_from_buffer are useful, I could better understand your position if you have something which you disagree with how the engine currently implements image loading, I'm just following existing development architecture/design really.

groud commented 4 years ago

That's more like comparing apples to oranges. My use case is more like for user-generated content. A regular player won't be able to create PCK files even if you provide them the tools to do this. Also, you'd have to consider #1212 because I don't believe the interface for this to be usable in practice, or it's just a lack of documentation, I don't know. The use case is already quite limited even for loading static images, mind you, but there's still support for that in Godot, which is great and perhaps I just got lucky enough to have those image loading methods in the first place.

As you may already know, I am still skeptical about having image manipulation tools accessible to users. This is a very limited use case that I generally would have preferred moved out to plugins. As everything has a maintenance cost in Godot, so the more tool we provide to the user, the more difficult it becomes to have an engine free of bugs.

So this make me even more skeptical about giving the user access to "sequence of images" manipulations, especially when you have other way to solve the problem easily.

I mean, unless this feature helps simplifying things in general, like removing another type of texture, or simplifying a lot other classes. I don't see it as an improvement, sorry. Flexibility has a cost, and if this flexibility has no use case, it's pointless.

Well, despite the limitations, I'm not sure whether it's reasonable to feature-creep those classes, I think they have their own purpose and they do the best thing they were initially created for.

If a new feature has a use case, this is not feature creep. And it's a lot simpler to change a little bit these classes than creating a whole new class to be used in several places in the engine.

@groud you haven't commented on AssetLib usage and whether you think that existing methods such as Image.load_png_from_buffer are useful, I could better understand your position if you have something which you disagree with how the engine currently implements image loading, I'm just following existing development architecture/design really.

I don't know, I'd say that as the image can be manipulated it makes sense to be able to able to load it from a file or a buffer. But well, as I said I am still skeptical about keeping image manipulation in the engine.

Edit: for the AssetLib, it might be usefull to have animated images yes, but I am not sure if we discuss the use case or the implementation. I think the feature is useful, but I don't think it needs to create a new class for it.

Xrayez commented 4 years ago

Thanks for expressing your fundemantal concerns @groud. To summarize our IRC discussion:

I wouldn't be interested in implementing this proposal without implementing ImageFrames class or alike (mainly because of load methods for animated images, that's the only feasible way to expose such a feature to scripting without touching other core classes).

If someone else is willing to implement this without adding ImageFrames class as concerned by @groud, I'd still be interested in the discussion because I need to know what features will be implemented in Godot 4.0, so I can properly make forward-compatible changes in the Goost module. That said, I'm not particularly against @groud's idea, but that just takes more (double) work that I'm not sure whether I'm willing to do myself at the moment, so this would have to wait up until 4.0 is usable enough in production, I'll be happy to elaborate/review other implementations in the meantime.

Far-fetched solution is adding support for animated image data to be directly in the Image class, but again that requires modifying core and you'd have to do more work, and so far one of the major concerns expressed is maintenance, and those kind of changes wouldn't be compatible with 3.2 anymore.


Regarding maintenance costs (Russian idiom):

И рыбку съесть, и косточкой не подавиться

Which more or less translates to:

You can't have your cake and eat it

Surely you have to maintain those features, else the engine will cease being useful, I suspect I wouldn't be trusted as a contributor to maintain those features as well. 😛

Warning: linking #575 to prevent this going off-topic.

Xrayez commented 4 years ago

Created alternative proposal for the sake of discussion: #1448. 🙂

Xrayez commented 4 years ago

I've made some changes to goostengine/goost#8, namely d404259 (#8). This allows to treat ImageFrames loading just like Image with a similar API, and of course it does allow to simplify importers in case more animated image formats get implemented.

So you see, this is why I also raised #1448 because this logic could be in theory merged directly within Image class, but since I have no clue/idea whether the implementation would be approved for core, I just work on the level of public API currently, and yeah this does "require" creating proxy classes like the proposed ImageFrames, I think there's no way around it. I mean, there's always a way but I rarely/almost never sign up for implementing incomplete solutions. It's not like there's only GIF for animated images out there.

See also https://github.com/goostengine/goost/pull/8#issuecomment-687928457.

Xrayez commented 4 years ago

Notifying that this feature is already available in Goost as implemented in goostengine/goost#8 (feel free to close this if no consensus is reached, I'm not particularly interested in providing concrete use cases because I'm already able to benefit from this, the only difference is that it's not implemented in Godot).

Also after re-reading discussion in this proposal, I'd like to additionally comment on previous statements in retrospective:

As everything has a maintenance cost in Godot, so the more tool we provide to the user, the more difficult it becomes to have an engine free of bugs.

Yeah, as of now, I'm willing to maintain those features there, obviously, I'm not afraid of work. I'm sure if more people find something useful, there will be a person who'd be capable/interested in maintaining those features. If not, the feature can be "commented out" until someone can actually resurrect it. 🙂

Also, the idea of "the engine which is free of bugs" is illusion, whenever you add new features, you'll almost always have a set of bugs which will likely be never resolved. Most of them are not critical, though.

But well, as I said I am still skeptical about keeping image manipulation in the engine.

This is likely not an issue, quoting reduz on this at #godotengine-devel IRC channel:

I am not against adding more image editing functions to Image to be honest, specially because in Godot 4.0 Image no longer requires locking to access a lot of effects in games that are difficult to do with textures may be doable easily in CPU with images, then uploaded to GPU, which is cheap in 4.0


So if I understand your point correctly, your issue is only to be able to load animated images like gifs at runtime ? Considering you can already have PCK files for modding, the use case seems quite limited no ? Or maybe I misunderstood something.

See #1632.

ChronoDK commented 3 years ago

I would love to see animated png support in Godot. Exporting files to an animated png from my animation tool would be much preferred, above exporting the individual frames as I do now. I hate looking at a folder with 100s of tiny images. There will be some limitations sure, but it would also bring a lot of simplification to the project. I could for instance use a regular png during devopment, and then simply replace it with the animated png when the artist has finished it.

Calinou commented 3 years ago

@ChronoDK The issue is that the library we use to decode PNGs doesn't support APNG: https://github.com/glennrp/libpng/issues/267

Xrayez commented 3 years ago

Seems like there's no consensus here according to discussion, I have to admit I'd still insist on the particular implementation I've made in goostengine/goost#8, otherwise I don't quite see the usefulness of implementing this proposal as-is.

If you don't agree with suggested implementation, please suggest alternatives. If not, I think it's safe to close this proposal.

Calinou commented 3 years ago

It would be nice to support GIFs as the backend of the asset library accepts those files as image previews. This way, they can display properly in the editor as well.

The same goes for APNG, since web browsers now support that format well. However, supporting APNG may be more difficult (see my comment above).

fire commented 3 years ago

Closed https://github.com/godotengine/godot/pull/31831#issuecomment-827132820 due to a weak position on advantages and a significant disadvantage for gifs import.

reduz commented 3 years ago

Likewise, you probably know at this point given this PR is old but this :

Describe the problem or limitation you are having in your project: Godot doesn't currently support loading animated images (such as GIF), and there are no core data structures to facilitate this. > Currently, we are limited to a single Image class which acts as a container for static images and provides basic image processing.

Does not answer the question on how it is a limitation being faced on an actual project, so IMO these type of proposals are not very valid in general and just ought to be ignored.

fire commented 3 years ago

Closed because of above reasoning by reduz.

fire commented 3 years ago

Please create a counter-proposal. Reduz doesn't want to be the final authority on this.

reduz commented 3 years ago

I would not close it, I mean maybe a better reason for doing this exists, but it should be something related to an actual project, not speculation.

Xrayez commented 3 years ago

I agree with @reduz here, it should not be part of core, so closing.

Goblinth commented 3 years ago

@ChronoDK The issue is that the library we use to decode PNGs doesn't support APNG: glennrp/libpng#267

From what I understand the libpng that Godot uses has this APNG patch applied to it already, which was added in this commit.

Xrayez commented 3 years ago

From what I understand the libpng that Godot uses has this APNG patch applied to it already, which was added in this commit.

Interesting, I wonder how easy it would be to make this available via modules.... Since Goost already support importing GIFs as AnimatedTextures, it should also be easier to recognize APNG format and define an importer for it.

For anyone interested, here's the GIF module implementation in Goost: https://github.com/goostengine/goost/tree/gd3/modules/gif. Animated loaders/importers could be moved to core/image/ and a new apng module could be created.

BlooRabbit commented 2 years ago

Use case: we are trying to include GIFs (or ANPG) in tutorials for our Kamaeru game. Videos are not convenient because Videoplayer is not looping (unless you use hacks). Spritesheets are possible but take up to much space when using lots of images (and subject to the 4096 limitation on mobiles).

Calinou commented 2 years ago

Videos are not convenient because Videoplayer is not looping (unless you use hacks).

To me, it sounds like implementing looping in VideoStreamPlayer makes more sense.