Skip to content

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Sep 16, 2025

Changes:

  • Add _request_update() to avoid calling _update_texture() more than once in a single frame.
  • Add force_update_texture() which applies the changes immediately if the texture needs to be updated.
  • Made some changes to avoid errors while updating the texture when some parameters are invalid.

@WhalesState WhalesState requested review from a team as code owners September 16, 2025 18:25
@clayjohn
Copy link
Member

The usual pattern we use is to defer the update to the end of the frame using call_deferred(). Is there any reason that won't work here?

Exposing a redundant feature for a small theoretical optimization isn't something we like to do

@WhalesState
Copy link
Contributor Author

WhalesState commented Sep 16, 2025

For example, If you have 100 icons for your game and you want to update their colors, scale and saturation when the theme changes it will emit changed signal 3 times for each texture instead of just once. All the setters do call DPITexture::_update_texture() which emits changed which calls TextureRect::_texture_changed() or Button::_texture_changed() and both of them calls queue_redraw() and update_minimum_size(). And to avoid that you will have to create 100 new textures instead of just being able to update them with minimal cost.

@clayjohn
Copy link
Member

I think you misunderstood my comment.

Our usual pattern is to have an internal setter like this:

void request_update() {
    if (update_requested) {
        return;
    }
    update_requested = true;
    call_deferred("update_props");
}

update_props would handle all the expensive parts of the update (including emitting changed and any other signals)

This ensures that the expensive parts are only called once per frame regardless of what updates you do.

@WhalesState
Copy link
Contributor Author

Ok I got it now, Thanks for making it clear.

@WhalesState WhalesState marked this pull request as draft September 16, 2025 19:19
@WhalesState WhalesState changed the title Add DPITexture::update Insure DPITexture::_update_texture only gets called once at the end of the frame. Sep 16, 2025
@WhalesState
Copy link
Contributor Author

WhalesState commented Sep 16, 2025

Implemented _request_update(). We may also need to add a way to allow users to call _update_texture() if they need the changes to be reflected immediately.

@WhalesState WhalesState marked this pull request as ready for review September 16, 2025 19:48
@AThousandShips AThousandShips changed the title Insure DPITexture::_update_texture only gets called once at the end of the frame. Ensure DPITexture::_update_texture only gets called once at the end of the frame. Sep 17, 2025
@AThousandShips AThousandShips added this to the 4.6 milestone Sep 17, 2025
@AThousandShips AThousandShips removed the request for review from a team September 17, 2025 07:54
@WhalesState WhalesState marked this pull request as draft September 19, 2025 12:21
@WhalesState
Copy link
Contributor Author

WhalesState commented Sep 19, 2025

To test the changes I have made DPITexture::_update_texture() prints "Internal (Texture is updated)" if update_requested is true and used this script.

@tool
extends Control


func _ready():
	var rect: TextureRect = TextureRect.new()
	add_child(rect)
	var svg: String = FileAccess.get_file_as_string("res://icon.svg")
	var tex: DPITexture = DPITexture.create_from_string(svg)
	rect.texture = tex

	print("create_from_string(svg)")
	print("\"Internal (Texture is updated)\" should print before \"create_from_string(svg)\"")
	print()
	
	tex.set_base_scale(0.5)
	print("set_base_scale(0.5)")
	tex.set_saturation(0.5)
	print("set_saturation(0.5)")
	tex.force_update_texture()
	print("force_update_texture()")
	print("\"Internal (Texture is updated)\" should print before \"force_update_texture()\"")
	print()

	tex.set_base_scale(0.5)
	print("set_base_scale(0.5)")
	tex.set_saturation(0.5)
	print("set_saturation(0.5)")
	tex.force_update_texture()
	print("force_update_texture()")
	print("\"Internal (Texture is updated)\" should not print (ressetting the same values shouldn't update)")
	print()
	
	tex.set_base_scale(0.2)
	print("set_base_scale(0.2)")
	tex.set_saturation(0.2)
	print("set_saturation(0.2)")
	tex.set_base_scale(4)
	print("set_base_scale(4)")
	tex.set_base_scale(2)
	print("set_base_scale(2)")
	tex.set_base_scale(1)
	print("set_base_scale(1)")
	print("\"Internal (Texture is updated)\" should print after this message (queued)")
	
	await get_tree().process_frame
	print()
	print("await get_tree().process_frame")
	print()
	
	tex.force_update_texture()
	print("force_update_texture()")
	tex.force_update_texture()
	print("force_update_texture()")
	print("\"Internal (Texture is updated)\" should not print (already updated)")
	
Internal (Texture is updated)
create_from_string(svg)
"Internal (Texture is updated)" should print before "create_from_string(svg)"

set_base_scale(0.5)
set_saturation(0.5)
Internal (Texture is updated)
force_update_texture()
"Internal (Texture is updated)" should print before "force_update_texture()"

set_base_scale(0.5)
set_saturation(0.5)
force_update_texture()
"Internal (Texture is updated)" should not print (ressetting the same values shouldn't update)

set_base_scale(0.2)
set_saturation(0.2)
set_base_scale(4)
set_base_scale(2)
set_base_scale(1)
"Internal (Texture is updated)" should print after this message (queued)
Internal (Texture is updated)

await get_tree().process_frame

force_update_texture()
force_update_texture()
"Internal (Texture is updated)" should not print (already updated)

@WhalesState WhalesState marked this pull request as ready for review September 19, 2025 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants