Add `texture_2d_update_partial` method #75892

Redwarx008 · 2023-04-10T14:54:09Z

Add texture_2d_update_partial method for RenderingServer.
Compressed textures are not supported, they are usually not present in use cases for this function.
~~The gles3 backend does not have a corresponding implementation (texture_set_data_partial() is called inside but it has been not implemented).~~
edit: Now gles3 backend is available, removed texture_set_data_partial() which was not implemented.
Bugsquad edit: This closes #65762.

Zylann · 2023-04-10T16:28:55Z

servers/rendering_server.h

@@ -107,6 +107,7 @@ class RenderingServer : public Object {
 	virtual RID texture_proxy_create(RID p_base) = 0;

 	virtual void texture_2d_update(RID p_texture, const Ref<Image> &p_image, int p_layer = 0) = 0;
+	virtual void texture_2d_update_partial(RID p_texture, const Ref<Image> &p_sub_image, int p_dst_x, int p_dst_y, int p_mipmap = 0, int p_layer = 0) = 0;


Performance: for cases where only a part of the source image are uploaded, it would be preferable if the method took a sub-region of the source image, rather than forcing to first instantiate another and copy pixels from the source image just to be passed to this function.
Like it was in Godot 3: https://docs.godotengine.org/en/3.6/classes/class_visualserver.html#class-visualserver-method-texture-set-data-partial
Though in Godot 4 the signature could be shortened using Rect2i and Vector2i.

it would be preferable if the method took a sub-region of the source image, rather than forcing to first instantiate and copy it from the source image just to be passed to this function.

I don't think there might be a big performance gap, and even if you do that, you're just calling Image.get_region() inside the method.

But why is it necessary to even do that? Is it a requirement of Vulkan/OpenGL? I think OpenGL required that, but not sure about Vulkan.

Not sure, but I think the current approach is more in line with function names.
Moreover, Image.get_data() must be called internally anyway. This method produces a complete copy operation, and calling this function may be expensive if the subimage is not obtained first.

I would tend to agree with Zylann here, I would add arguments for source coordinates. Godot 3.x had the same and the currently unimplemented stub also has signature like that:

godot/drivers/gles3/storage/texture_storage.cpp

Lines 1276 to 1278 in 578ca94

void TextureStorage::texture_set_data_partial(RID p_texture, const Ref<Image> &p_image, int src_x, int src_y, int src_w, int src_h, int dst_x, int dst_y, int p_dst_mip, int p_layer) {

ERR_PRINT("Not implemented yet, sorry :(");

}

Implementing that can be slightly more difficult as you also have to calculate image stride and offsets, but it should be worth it in performance. My ultimate version (no need to implement) would probably take a PackedByteArray instead of Image, similar to high-performance mesh update methods like mesh_surface_update_vertex_region.

Also note that Image.get_data() returns a Vector, which is a copy-on-write container. It doesn't copy the contents until they are modified, which in this case is not needed.

Also, thanks for doing this! This will be a very useful method to many.

I would tend to agree with Zylann here, I would add arguments for source coordinates. Godot 3.x had the same and the currently unimplemented stub also has signature like that:

godot/drivers/gles3/storage/texture_storage.cpp

Lines 1276 to 1278 in 578ca94

void TextureStorage::texture_set_data_partial(RID p_texture, const Ref<Image> &p_image, int src_x, int src_y, int src_w, int src_h, int dst_x, int dst_y, int p_dst_mip, int p_layer) {

ERR_PRINT("Not implemented yet, sorry :(");

}

Implementing that can be slightly more difficult as you also have to calculate image stride and offsets, but it should be worth it in performance. My ultimate version (no need to implement) would probably take a PackedByteArray instead of Image, similar to high-performance mesh update methods like mesh_surface_update_vertex_region.

Also note that Image.get_data() returns a Vector, which is a copy-on-write container. It doesn't copy the contents until they are modified, which in this case is not needed.

Also, thanks for doing this! This will be a very useful method to many.

That makes sense, I've updated the api.

virtual void texture_2d_update_partial(RID p_texture, const Ref<Image> &p_src_image, Rect2i p_region, Vector2i p_dst, int p_mipmap = 0, int p_layer = 0)

I think the region should still be kept as small as possible. For example, rgb8 is rendered as rgba8 in vulkan, a Image.convert() will be called inside, which is a very expensive operation.

clayjohn

Overall this looks great and is a much needed missing feature in Godot 4.0. I left a few comments on things that need to be fixed before merging though.

Please let me know if you have any questions or if anything doesn't make sense

clayjohn · 2023-06-13T18:49:13Z

drivers/gles3/storage/texture_storage.cpp

@@ -812,6 +812,41 @@ void TextureStorage::texture_2d_update(RID p_texture, const Ref<Image> &p_image,
 #endif
 }

+void TextureStorage::texture_2d_update_partial(RID p_texture, const Ref<Image> &p_src_image, Rect2i p_region, Vector2i p_dst, int p_mipmap, int p_layer) {
+	ERR_FAIL_COND(p_src_image.is_null() || p_src_image->is_empty());
+	ERR_FAIL_COND_MSG(p_src_image->is_compressed(), "Compressed image is not supported");


Suggested change

ERR_FAIL_COND_MSG(p_src_image->is_compressed(), "Compressed image is not supported");

ERR_FAIL_COND_MSG(p_src_image->is_compressed(), "Compressed image is not supported for partial texture updates. Please use an uncompressed image instead");

On this note, it looks like the check is missing for if p_texture is compressed. We should also fail if p_texture is a compressed format. Alternatively, we can add support for compressed formats as we have in 3.x

clayjohn · 2023-06-13T18:55:31Z

drivers/gles3/storage/texture_storage.cpp

+	glBindTexture(texture->target, texture->tex_id);
+
+	int pixel_size = img->get_format_pixel_size(img->get_format());
+	int ofs = (p_region.position.x + p_region.position.y * p_src_image->get_width()) * pixel_size;


This isn't going to work if the src_image width is different from region width. You will end up getting the wrong data for all pixels except for the first row because openGL is going to pull from the data array without regard to the region size. in other words, it expects that you will have packed the data into an array already.

We have to give OpenGL a tightly packed array. Which means the size of the region given to glTexSubImage2D must match the size of the image used to blit.

Accordingly, we need to handle the case where p_region size does not match p_src_image size. We can do that the same way we did in 3.x. I.e. if the region size is not the same as the src_image size, then we copy the partial rect out of the src image and use that. If the region size matches, then we can use it directly with no copy. Performance-minded uses can thus always provide the correctly-sized image to avoid the expensive copy.

clayjohn · 2023-06-13T19:01:34Z

servers/rendering/renderer_rd/storage_rd/texture_storage.cpp

+	if (src_format == Image::FORMAT_RGB8) {
+		validated = p_src_image->get_region(p_region);
+		validated->convert(Image::FORMAT_RGBA8);


Why are you converting to an alpha format? If it is to enforce 4-byte alignment, see how _texture_update() handles it. You should be able to copy that approach directly.

We should really avoid doing a full texture convert in such cases.

AThousandShips · 2023-06-14T09:51:02Z

Needs a rebase to pass CI

Edit: I'm sorry it seems the CI was not fixed, waiting for a new fix

KoBeWi · 2023-07-21T14:58:57Z

doc/classes/RenderingDevice.xml

+			<param index="5" name="layer" type="int" default="0" />
+			<param index="6" name="post_barrier" type="int" enum="RenderingDevice.BarrierMask" default="7" />
+			<description>
+			</description>


Needs description.
Same below.

KoBeWi · 2023-07-21T14:59:54Z

Not sure what is the status of this PR, but I can confirm that the feature works correctly (even if implementation isn't perfect apparently?).

Zireael07 · 2023-08-02T06:23:46Z

Why close?

Redwarx008 · 2023-08-02T11:52:52Z

Why close?

Because of some operational mistakes, I have resubmitted a pr.#80164

Redwarx008 requested a review from a team as a code owner April 10, 2023 14:54

YuriSizov mentioned this pull request Apr 10, 2023

Add texture_2d_update_partial #75882

Closed

YuriSizov added enhancement topic:rendering labels Apr 10, 2023

YuriSizov modified the milestones: 4.1, 4.x Apr 10, 2023

Redwarx008 changed the title ~~Add texture_2d_partial_update method~~ Add texture_2d_update_ partial method Apr 10, 2023

Zylann reviewed Apr 10, 2023

View reviewed changes

Redwarx008 force-pushed the add-texture-partial-update branch from a26e04b to 75807bc Compare April 11, 2023 03:06

Redwarx008 requested a review from a team as a code owner April 11, 2023 03:06

Redwarx008 force-pushed the add-texture-partial-update branch from 75807bc to 28523cf Compare April 11, 2023 07:25

Redwarx008 changed the title ~~Add texture_2d_update_ partial method~~ Add texture_2d_update_ partial method Apr 11, 2023

SlashScreen mentioned this pull request Apr 14, 2023

Implement Terrain godotengine/godot-proposals#6121

Open

Redwarx008 force-pushed the add-texture-partial-update branch 10 times, most recently from 97aa269 to 2f54340 Compare April 29, 2023 07:27

clayjohn reviewed Jun 13, 2023

View reviewed changes

clayjohn modified the milestones: 4.x, 4.2 Jun 13, 2023

Redwarx008 force-pushed the add-texture-partial-update branch from 2f54340 to f66dba1 Compare June 14, 2023 08:08

Redwarx008 force-pushed the add-texture-partial-update branch from f66dba1 to 0f35461 Compare June 14, 2023 11:26

Redwarx008 force-pushed the add-texture-partial-update branch 3 times, most recently from ecae02b to d441469 Compare June 15, 2023 06:18

KoBeWi reviewed Jul 21, 2023

View reviewed changes

AThousandShips changed the title ~~Add texture_2d_update_ partial method~~ Add texture_2d_update_partial method Jul 21, 2023

Redwarx008 force-pushed the add-texture-partial-update branch 2 times, most recently from d97c49b to cd1a6ec Compare August 2, 2023 04:29

Redwarx008 closed this Aug 2, 2023

Redwarx008 force-pushed the add-texture-partial-update branch 2 times, most recently from cd1a6ec to ba3fb66 Compare August 2, 2023 04:30

akien-mga added archived salvageable labels Aug 2, 2023

akien-mga removed the salvageable label Aug 2, 2023

clayjohn removed this from the 4.2 milestone Aug 2, 2023

akien-mga mentioned this pull request Aug 2, 2023

Add texture partial update method #80164

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `texture_2d_update_partial` method #75892

Add `texture_2d_update_partial` method #75892

Redwarx008 commented Apr 10, 2023 •

edited

Loading

Zylann Apr 10, 2023 •

edited

Loading

Redwarx008 Apr 10, 2023

Zylann Apr 10, 2023 •

edited

Loading

Redwarx008 Apr 11, 2023 •

edited

Loading

bitsawer Apr 27, 2023 •

edited

Loading

Redwarx008 Apr 28, 2023

Redwarx008 Apr 28, 2023 •

edited

Loading

clayjohn left a comment

clayjohn Jun 13, 2023

clayjohn Jun 13, 2023

clayjohn Jun 13, 2023

AThousandShips commented Jun 14, 2023 •

edited

Loading

KoBeWi Jul 21, 2023

KoBeWi commented Jul 21, 2023

Zireael07 commented Aug 2, 2023

Redwarx008 commented Aug 2, 2023

	void TextureStorage::texture_set_data_partial(RID p_texture, const Ref<Image> &p_image, int src_x, int src_y, int src_w, int src_h, int dst_x, int dst_y, int p_dst_mip, int p_layer) {
	ERR_PRINT("Not implemented yet, sorry :(");
	}

	ERR_FAIL_COND_MSG(p_src_image->is_compressed(), "Compressed image is not supported");
	ERR_FAIL_COND_MSG(p_src_image->is_compressed(), "Compressed image is not supported for partial texture updates. Please use an uncompressed image instead");

Add texture_2d_update_partial method #75892

Add texture_2d_update_partial method #75892

Conversation

Redwarx008 commented Apr 10, 2023 • edited Loading

Zylann Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Redwarx008 Apr 10, 2023

Choose a reason for hiding this comment

Zylann Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Redwarx008 Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

bitsawer Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Redwarx008 Apr 28, 2023

Choose a reason for hiding this comment

Redwarx008 Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

clayjohn left a comment

Choose a reason for hiding this comment

clayjohn Jun 13, 2023

Choose a reason for hiding this comment

clayjohn Jun 13, 2023

Choose a reason for hiding this comment

clayjohn Jun 13, 2023

Choose a reason for hiding this comment

AThousandShips commented Jun 14, 2023 • edited Loading

KoBeWi Jul 21, 2023

Choose a reason for hiding this comment

KoBeWi commented Jul 21, 2023

Zireael07 commented Aug 2, 2023

Redwarx008 commented Aug 2, 2023

Add `texture_2d_update_partial` method #75892

Add `texture_2d_update_partial` method #75892

Redwarx008 commented Apr 10, 2023 •

edited

Loading

Zylann Apr 10, 2023 •

edited

Loading

Zylann Apr 10, 2023 •

edited

Loading

Redwarx008 Apr 11, 2023 •

edited

Loading

bitsawer Apr 27, 2023 •

edited

Loading

Redwarx008 Apr 28, 2023 •

edited

Loading

AThousandShips commented Jun 14, 2023 •

edited

Loading