Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't request redraw in AnimatedTexture to decrease editor CPU/GPU usage #61943

Closed

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 11, 2022

This means AnimatedTextures will update at an erratic rate if nothing else is causing the editor to redraw continuously (or projects, if Low Processor Mode is enabled). However, lowering the CPU/GPU usage by a significant amount is probably preferred in most situations.

This will not affect the editor if Update Continuously is enabled, or if Low Processor Mode is disabled in the project (which is the default).

This partially addresses #39758.

PS: I wonder why the old code isn't forcing a redraw request here instead:

VisualServer::get_singleton()->texture_set_proxy(proxy, frames[current_frame].texture->get_rid());

This would lead to lower CPU/GPU usage already, butit wouldn't fully resolve the original issue (especially at higher animation FPS).

This means AnimatedTextures will update at an erratic rate if nothing
else is causing the editor to redraw continuously (or projects,
if Low Processor Mode is enabled).

This will not affect the editor if Update Continuously is enabled,
or if Low Processor Mode is disabled in the project (which is the default).
@lawnjelly
Copy link
Member

lawnjelly commented Jun 12, 2022

One problem is that there are lots of similar cases to this that will cause redraw in 4.x, and it is impossible to know in advance which users will want to cause updates and which not. You could end up swapping this to not continuously update, then a user will complain that it doesn't continuously update, make another PR to flip it back in the source, repeat ad infinitum.

You could make it controllable per feature but this is probably overkill.

Overall my recommended approach for this use case (laptops / passive cooled / battery saving / low power machines that struggle to render editor) would be to port #53463 to 4.x. I think reduz was unsure about the vital updates features at the time in master (maybe thinking that a low update rate would work as well), but the feature works great in 3.x. Perhaps we will get more feedback on this after 3.5 as many are unaware of this feature as yet.)

PS: I wonder why the old code isn't forcing a redraw request here instead:

I suspect this is because the redraw_if_visible flag is expected to handle all this automagically. It may not currently, but that is probably a better approach to have the client code thin, instead of writing this in multiple places, and have the VisualServer respect things like the update rate. This would likely make it easier to write client code and less bug prone.

To get animated_texture to respect pause, we presumably could do something like this:

void AnimatedTexture::set_pause(bool p_pause) {
	RWLockWrite r(rw_lock);
	if (p_pause != pause) {
		pause = p_pause;
		VisualServer::get_singleton()->texture_set_force_redraw_if_visible(proxy, !pause);
	}
}

Although you would have to check that was ok with the lock as I'm not familiar with RWLockWrite. I didn't fix this in #61965 (as I'm only concerned with vital update mode there), so feel free to investigate the above.

@reduz
Copy link
Member

reduz commented Aug 6, 2022

Again I dont think this is a problem. For a lot of users, more CPU usage is not a problem as long as things work properly, you should not break things for them. We could see a special mode for laptops or something like that, but this is not the right way.

@Calinou
Copy link
Member Author

Calinou commented Aug 7, 2022

Closing in favor of #53463, which should be forward-ported to master eventually.

@Calinou Calinou closed this Aug 7, 2022
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