Add texture partial update method #80164

Redwarx008 · 2023-08-02T11:51:46Z

Due to some operational errors, I had to resubmit the pr.
Add texture_2d_update_partial method for RenderingServer.
Compressed textures are not supported, they are usually not present in use cases for this function.

Bugsquad edit:

akien-mga · 2023-08-02T11:54:30Z

Your branch is a couple of months behind the current master, and thus has merge conflicts. Could you rebase?

Edit: I see you pushed a merge commit at the same time I wrote this. Please rebase instead to keep a single commit in the PR. See PR workflow.

Redwarx008 · 2023-08-22T02:28:54Z

Reference #80781, fixed this issue

clayjohn

It looks like you have lost a few of the changes you made in the last PR, particularly, it looks like this has lost the ability to specify a sub region of the source image

clayjohn · 2023-08-22T12:44:27Z

drivers/gles3/storage/texture_storage.cpp

+	ERR_FAIL_COND(p_data.is_null() || p_data->is_empty());
+
+	Texture *texture = texture_owner.get_or_null(p_texture);
+	ERR_FAIL_COND_MSG(p_data->is_compressed() || texture->compressed, "Compressed texture is not supported.");


Suggested change

ERR_FAIL_COND_MSG(p_data->is_compressed() || texture->compressed, "Compressed texture is not supported.");

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

clayjohn · 2023-08-22T12:44:41Z

servers/rendering/renderer_rd/storage_rd/texture_storage.cpp


+	Texture *texture = texture_owner.get_or_null(p_texture);
+	ERR_FAIL_COND_MSG(p_data->is_compressed() || texture->format > Image::FORMAT_RGBE9995, "Compressed texture is not supported.");


Suggested change

ERR_FAIL_COND_MSG(p_data->is_compressed() || texture->format > Image::FORMAT_RGBE9995, "Compressed texture is not supported.");

ERR_FAIL_COND_MSG(p_data->is_compressed() || texture->format > Image::FORMAT_RGBE9995, "Compressed texture is not supported for partial texture updates. Please use an uncompressed image instead");

Redwarx008 · 2023-08-22T13:20:34Z

It looks like you have lost a few of the changes you made in the last PR, particularly, it looks like this has lost the ability to specify a sub region of the source image

After thinking, I changed the api, now the parameter data:Image is a wrapper for the target area data. This makes the api clearer.

Redwarx008 · 2023-08-22T13:28:12Z

If the texture is updated from a specific sub region of the src image, when the texture is three-channel, it will be converted to four-channel. This is very expensive if the texture is large

npip99 · 2023-11-23T01:29:40Z

Can I know what's holding this PR back? I can try to help contribute; it was rebased but never merged.

There's no solution or alternative to this functionality not existing; there's just no way to edit terrains or modify images without FPS dropping significantly when updating the entire image.

AThousandShips · 2023-11-23T12:04:21Z

If hasn't been approved so it can't be merged, we are currently in feature freeze and this can't be considered before 4.2 has been released

Calinou · 2023-11-23T13:28:35Z

@npip99 I suggest testing this pull request and uploading a testing project that makes use of it to demonstrate it's working.

KoBeWi · 2023-11-23T13:54:46Z

We are using this PR in a custom fork for a new project (with terrain destruction like in Lumencraft, but in Godot 4), so I can confirm it works.

clayjohn

Looks like this PR lost some of the work done in #75892

We should get the API back to the state it was in before (i.e. users should be able to specify a sub-region). There is no performance benefit to removing that ability from the API. The code can easily do the following:

If the sub-region is the size of p_image, copy directly (as you have now)

If the sub-region is smaller than p_image, grab a slice of the image (like we did in 3.x)

godot/drivers/gles3/rasterizer_storage_gles3.cpp

Lines 965 to 968 in e96c42f

    
           Ref<Image> p_sub_img = p_image; 
        
           if (src_x > 0 || src_y > 0 || src_w != p_image->get_width() || src_h != p_image->get_height()) { 
        
           	p_sub_img = p_image->get_rect(Rect2(src_x, src_y, src_w, src_h)); 
        
           }

If the sub-region is larger than p_image, error

If we remove the sub-region from the API, we just force the user to do step 2. We don't actually save any performance and we make the API harder for users to use. Further, users will see the change as a regression, because something that was easy to do in 3.x is now hard

clayjohn · 2023-11-23T16:41:33Z

servers/rendering_server.h

@@ -109,6 +109,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_data, Vector2i p_dst_pos, int p_dst_mipmap = 0, int p_layer = 0) = 0;


Just linking back to the previous PR's reviews #75892 (comment)

It seems like you have lost some of the API changes suggested by reviewers.

Redwarx008 · 2023-11-24T01:35:16Z

Looks like this PR lost some of the work done in #75892

This is done intentionally and is not some work lost.
Actually, a three-channel texture is not available, so here it will be converted into a four-channel texture. If subregions are used, the entire p_image will be converted, which has a significant impact on performance if its dimensions are very large. We can perform special processing for all three-channel textures here, but this is a hack. We can avoid this situation by forcing the user to obtain the sub-area outside first in the design of the API.
Many APIs have changed from 3.x to 4.x, why not include this? What's more, it is not a commonly used API.

npip99 · 2023-11-25T02:55:01Z

Why would the whole p_image have to be converted? If the src_rect is 10x10, but the src_image is 2048x2048, can't it be possible to just copy and convert the data from the 10x10 source subregion, without copying or converting the entire 2048x2048 image.

We can avoid this situation by forcing the user to obtain the sub-area outside first in the design of the API.

That's true, the user can call Image.get_region in GDScript, and then pass that into the new function signature. But, it might be possible to write code that makes texture_2d_update_partial(src_image, src_rect) faster than in GDScript calling Image.get_region and passing that. Given that it's a low-level function given exclusively for acceleration and speed, it would be useful to leave every opportunity available for it to be as efficient as it can be.

In my case, I'm keeping an "Image" instance in GDScript and editing it every single frame, and then I simply wish to blit a subrectangle back to the ImageTexture every single frame so that the 3D Model's texture updates in realtime. I personally track and update the minX/maxX/minY/maxY for every pixel update, so that I can give the src_rect during the blits. In this case, even if src_rect as an argument isn't more efficient (yet), it's also a much simpler API for this type of usage. I imagine my usage is in-general the most common usage for this exact function.

I think for now, with the current solution, it should be possible to just call Image.get_region internally, making it no faster than the user doing GDScript Image.get_region, but it also shouldn't be any slower either. And leave any future optimizations to the future (Any future optimizations wouldn't affect the Big-O or anything fundamental, it would just be slightly faster). In my case, I'd appreciate the opportunity for optimization in a future PR to be beneficial, as every incremental slight improvement allows me to increase the resolution of the modifiable texture in my game.

Redwarx008 · 2023-11-25T11:09:15Z

Why would the whole p_image have to be converted? If the src_rect is 10x10, but the src_image is 2048x2048, can't it be possible to just copy and convert the data from the 10x10 source subregion, without copying or converting the entire 2048x2048 image.

Since this only needs to be done for all three-channel textures, the special processing is inelegant.

nackata · 2024-07-06T13:16:45Z

Hello, this issue also can be fixed by this PR! Zylann/godot_heightmap_plugin#445

Redwarx008 requested review from a team as code owners August 2, 2023 11:51

Redwarx008 mentioned this pull request Aug 2, 2023

Add texture_2d_update_partial method #75892

Closed

akien-mga added enhancement topic:rendering labels Aug 2, 2023

akien-mga added this to the 4.2 milestone Aug 2, 2023

Redwarx008 force-pushed the add-texture-partial-update branch 17 times, most recently from d94a0cc to 316e86b Compare August 4, 2023 17:01

TokisanGames mentioned this pull request Aug 5, 2023

Implement GPU Painting TokisanGames/Terrain3D#174

Open

bitsawer mentioned this pull request Aug 19, 2023

Fix Vulkan texture update #80781

Merged

Redwarx008 force-pushed the add-texture-partial-update branch 3 times, most recently from d75283b to 30aed6b Compare August 22, 2023 03:10

Redwarx008 force-pushed the add-texture-partial-update branch 2 times, most recently from 7f07049 to 46003e4 Compare August 22, 2023 11:45

clayjohn reviewed Aug 22, 2023

View reviewed changes

Add texture partial update method

6864e88

Redwarx008 force-pushed the add-texture-partial-update branch from 46003e4 to 6864e88 Compare August 22, 2023 13:34

AThousandShips modified the milestones: 4.2, 4.3 Oct 31, 2023

clayjohn reviewed Nov 23, 2023

View reviewed changes

clayjohn modified the milestones: 4.3, 4.4 Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add texture partial update method #80164

Add texture partial update method #80164

Redwarx008 commented Aug 2, 2023 •

edited by akien-mga

Loading

akien-mga commented Aug 2, 2023 •

edited

Loading

Redwarx008 commented Aug 22, 2023

clayjohn left a comment

clayjohn Aug 22, 2023

clayjohn Aug 22, 2023

Redwarx008 commented Aug 22, 2023 •

edited

Loading

Redwarx008 commented Aug 22, 2023 •

edited

Loading

npip99 commented Nov 23, 2023

AThousandShips commented Nov 23, 2023

Calinou commented Nov 23, 2023 •

edited

Loading

KoBeWi commented Nov 23, 2023

clayjohn left a comment

clayjohn Nov 23, 2023

Redwarx008 commented Nov 24, 2023 •

edited

Loading

npip99 commented Nov 25, 2023 •

edited

Loading

Redwarx008 commented Nov 25, 2023

nackata commented Jul 6, 2024

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


		Texture *texture = texture_owner.get_or_null(p_texture);
		ERR_FAIL_COND_MSG(p_data->is_compressed() \|\| texture->format > Image::FORMAT_RGBE9995, "Compressed texture is not supported.");

	Ref<Image> p_sub_img = p_image;
	if (src_x > 0 \|\| src_y > 0 \|\| src_w != p_image->get_width() \|\| src_h != p_image->get_height()) {
	p_sub_img = p_image->get_rect(Rect2(src_x, src_y, src_w, src_h));
	}

Add texture partial update method #80164

Are you sure you want to change the base?

Add texture partial update method #80164

Conversation

Redwarx008 commented Aug 2, 2023 • edited by akien-mga Loading

akien-mga commented Aug 2, 2023 • edited Loading

Redwarx008 commented Aug 22, 2023

clayjohn left a comment

Choose a reason for hiding this comment

clayjohn Aug 22, 2023

Choose a reason for hiding this comment

clayjohn Aug 22, 2023

Choose a reason for hiding this comment

Redwarx008 commented Aug 22, 2023 • edited Loading

Redwarx008 commented Aug 22, 2023 • edited Loading

npip99 commented Nov 23, 2023

AThousandShips commented Nov 23, 2023

Calinou commented Nov 23, 2023 • edited Loading

KoBeWi commented Nov 23, 2023

clayjohn left a comment

Choose a reason for hiding this comment

clayjohn Nov 23, 2023

Choose a reason for hiding this comment

Redwarx008 commented Nov 24, 2023 • edited Loading

npip99 commented Nov 25, 2023 • edited Loading

Redwarx008 commented Nov 25, 2023

nackata commented Jul 6, 2024

Redwarx008 commented Aug 2, 2023 •

edited by akien-mga

Loading

akien-mga commented Aug 2, 2023 •

edited

Loading

Redwarx008 commented Aug 22, 2023 •

edited

Loading

Redwarx008 commented Aug 22, 2023 •

edited

Loading

Calinou commented Nov 23, 2023 •

edited

Loading

Redwarx008 commented Nov 24, 2023 •

edited

Loading

npip99 commented Nov 25, 2023 •

edited

Loading