godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Add Spritesheet support to RichTextLabel BBCode #3441

Closed progsource closed 2 years ago

progsource commented 3 years ago

Describe the project you are working on

I'm working on an adventure game. Nothing available public yet. Also like to do gamejams with Godot.

Describe the problem or limitation you are having in your project

In RichTextLabel it is currently possible to add images. Now in my adventure game there will be many icons that I would like to show in RichTextLabel, but I don't want to create an asset per icon, because I have them already all in one image.

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

Add support for spritesheets in BBCode. This way it would be possible to use one image with regions to select an icon to show in RichTextLabel

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

[img=<width>x<height> regionposx=<x> regionposy=<y> regionsize=<width>x<height>]{path}[/img]

Names of parameters just as example..

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

I'm not aware of a workaround other than to use many images.

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

Parsing of RichTextLabel happens in a private method... It could be made as an own thing, but that would, I guess, require to copy the whole RichTextLabel into an addon/module.

YuriSizov commented 3 years ago

I'm not aware of a workaround other than to use many images.

You can use any texture resource there. So you could use an atlas to drive all your images.

progsource commented 3 years ago

I'm not aware of a workaround other than to use many images.

You can use any texture resource there. So you could use an atlas to drive all your images.

So would I have to create many textures then in that case?

YuriSizov commented 3 years ago

I guess, though maybe there is a way to organize it better. Either way, your source texture would be just one asset in that case.

progsource commented 3 years ago

For me that sounds still like I have to do a lot of manual work to display the icons then. Even though making multiple textures might help it rendering-wise and image-wise, it doesn't solve the issue of having many assets (in that case then textures).

KoBeWi commented 3 years ago

There is a workaround that doesn't require lots of asset files, but it's only possible via scripts and you can't use bbcodes normally. Basically, the add_image() method takes a Texture parameter. Which means you can use a helper method to create a texture dynamically and then add it. E.g. you have your icons in a spritesheet, you'd use your function like add_image(get_spritesheet_frame(sheet, framex, framey)), and the actual function could be something like

func get_spritesheet_frame(texture, x, y):
    image = AtlasTexture.new()
    image.atlas = texture
    image.region = Rect2(x * 32, y * 32, 32, 32)
    return image

EDIT: If you want a bbcode workaround, you could use RichTextEffect. It allows you to define a custom tag, so make an "effect" that will display an image from spritesheet 🙃

progsource commented 3 years ago

If it is possible with a custom tag, then it would be nice to have it in the docs, I guess. I just don't get yet how to add an image on CharFXTransform.

KoBeWi commented 3 years ago

You can't add image normally. You can have a variable that stores a list of images you "added", use the effect callback to determine their position and draw them using _draw() method. This is a big hack, but should be possible to do (as a workaround, that is). I'll try to create an example.

KoBeWi commented 3 years ago

Uhhh I doubt this is useful xd ImageHack.tscn.txt (this is a self-contained scene, with dependency on icon.png)

For this text

Spritesheet test
Image 1 goes here -> [sheet image=icon.png size=32,32 frame=0]      [/sheet] <- image2 goes here -> [sheet image=icon.png size=32,32 frame=1] [/sheet]

And there the third one [sheet image=icon.png size=32,32 frame=2] [/sheet]

It displays image

Looks like absolute_index doesn't take newlines into account, but this is possible to workaround too. That solution is extremely inefficient though. A proper spritesheet support would be indeed better.

progsource commented 3 years ago

With the following code change/spike based on 3.3.4-stable, I can add images with regions:

diff --git a/scene/gui/rich_text_label.cpp b/scene/gui/rich_text_label.cpp
// [...]
                        push_meta(url);
                        pos = brk_end + 1;
                        tag_stack.push_front("url");
-               } else if (tag == "img") {
+               } else if (tag.begins_with("img ") || tag == "img") {

                        int end = p_bbcode.find("[", brk_end);
                        if (end == -1)
                                end = p_bbcode.length();

+                       bool is_region_enabled = false;
+                       Rect2 region;
+                       String params_s = tag.substr(3, tag.length());
+                       Vector<String> params = params_s.split(" ", false);
+
+                       for (int i = 0; i < params.size(); ++i) {
+                               int pos = params[i].find("region_x=");
+                               if (pos != -1) {
+                                       region.position.x = params[i].substr(pos + String("region_x=").length(), params[i].length()).to_double();
+                                       is_region_enabled = true;
+                                       continue;
+                               }
+
+                               pos = params[i].find("region_y=");
+                               if (pos != -1) {
+                                       region.position.y = params[i].substr(pos + String("region_y=").length(), params[i].length()).to_double();
+                                       is_region_enabled = true;
+                                       continue;
+                               }
+
+                               pos = params[i].find("region_w=");
+                               if (pos != -1) {
+                                       region.size.x = params[i].substr(pos + String("region_w=").length(), params[i].length()).to_double();
+                                       is_region_enabled = true;
+                                       continue;
+                               }
+
+                               pos = params[i].find("region_h=");
+                               if (pos != -1) {
+                                       region.size.y = params[i].substr(pos + String("region_h=").length(), params[i].length()).to_double();
+                                       is_region_enabled = true;
+                               }
+                       }
+
                        String image = p_bbcode.substr(brk_end + 1, end - brk_end - 1);

                        Ref<Texture> texture = ResourceLoader::load(image, "Texture");
                        if (texture.is_valid())
+                               if (is_region_enabled) {
+                                       Ref<AtlasTexture> atlas_tex = memnew(AtlasTexture);
+                                       atlas_tex->set_atlas(texture);
+                                       atlas_tex->set_region(region);
+                                       texture = atlas_tex;
+                               }
                                add_image(texture);

                        pos = end;
-                       tag_stack.push_front(tag);
+                       tag_stack.push_front("img");
                } else if (tag.begins_with("img=")) {

                        int width = 0;

I just am not sure about creating a PR yet, because of namings, code style, code optimization, if it's wanted etc. And it probably also should then be added to the block that is starting with } else if (tag.begins_with("img=")) {, I guess.

[img region_w=16 region_h=16 region_x=16]res://icon.png[/img]la di da[img region_w=16 region_h=16]res://icon.png[/img]

la la ->[img region_w=16 region_h=16 region_y=16]res://icon.png[/img]<-

richtextlabel

So for now I just leave this piece of code here for anybody who is interested 😉

progsource commented 3 years ago

Version based on master ( cacfb22 )

                        Ref<Texture2D> texture = ResourceLoader::load(image, "Texture2D");
                        if (texture.is_valid()) {
+                               Rect2 region;
+                               OptionMap::Element *region_option = bbcode_options.find("region");
+                               if (region_option) {
+                                       Vector<String> region_values = region_option->value().split(",", false);
+                                       if (region_values.size() == 4) {
+                                               region.position.x = region_values[0].to_float();
+                                               region.position.y = region_values[1].to_float();
+                                               region.size.x = region_values[2].to_float();
+                                               region.size.y = region_values[3].to_float();
+                                       }
+                               }
+
                                Color color = Color(1.0, 1.0, 1.0);
                                OptionMap::Element *color_option = bbcode_options.find("color");
                                if (color_option) {
// [...]
                                        }
                                }

+                               if (!region.has_no_area()) {
+                                       Ref<AtlasTexture> atlas_tex = memnew(AtlasTexture);
+                                       atlas_tex->set_atlas(texture);
+                                       atlas_tex->set_region(region);
+                                       texture = atlas_tex;
+                               }
+
                                add_image(texture, width, height, color, (InlineAlign)align);
                        }

Which would result in bbcode like [img region=0,0,32,32]res://icon.png[/img]

I think this looks more clean than my previous spike.

progsource commented 2 years ago

Regarding bbcode API - what would be preferred?

or something else?

progsource commented 2 years ago

ping? ;)

would love to add it, so I don't have to update my own RichTextLabel every time I update Godot

Calinou commented 2 years ago

ping? ;)

would love to add it, so I don't have to update my own RichTextLabel every time I update Godot

Feel free to open a pull request for this against the master branch :slightly_smiling_face:

Features need to be added to master first before they can be added to 3.x as well. This is to ensure feature parity for all features that are added to 3.x (and avoid functional regressions when people upgrade to Godot 4).

progsource commented 2 years ago

So shall I just pick one of the possible APIs? ( I tend to the [img region=0,0,32,32]res://icon.png[/img] version)

Calinou commented 2 years ago

So shall I just pick one of the possible APIs? ( I tend to the [img region=0,0,32,32]res://icon.png[/img] version)

This one sounds good to me. It needs to be concise after all, as I suppose referencing a spritesheet position is something you'll do often. Rect2() also has a similar x, y, width, height constructor.