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

Add editor vital redraws only option #53463

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Oct 6, 2021

When editor continuous redraws is switched off, the editor only redraws when a redraw_request was issued by an element in the scene. This works well in most situations, but when scenes contain dynamic content they will continuously issue redraw_requests.

This can be fine on high power desktops but can be an annoyance on lower power machines / laptops where it will eat battery.

This PR splits redraw requests into high and low priority requests, defaulting to high priority. Requests due to e.g. shaders using TIME are assigned low priority.

An extra editor setting is used to record the user preference and an extra option is added to the editor spinner menu, to allow the user to select between 3 modes:

  • Update Continuously
  • Update All Changes
  • Update Vital Changes

Fixes #16397
Implements godotengine/godot-proposals#942
See for instance #40499

blubits_vital
(There is no requirement to have a quick changeable setting in the spinner menu, I've added this for discussion, but it would also be perfectly feasible to have the editor settings option only.)

Notes

  • We can do a similar PR for 4.0.
  • I had been meaning to make such a PR for a while now, it is particularly important for those such as myself who have a low power / fanless PC. I usually have to close background scene tabs as a hack to prevent continuous updates, this PR will make life much easier.
  • This is also particularly useful for those people whose GPU etc struggles drawing the editor.
  • This can also make the editor more responsive. If it is taking, say 1/10th second to draw an update, it cannot respond to keypresses during this period, and therefore lags. By not doing these background updates, it means the editor is ready to respond immediately to keypresses etc.
  • Ideally it would be nice to just store this editor setting as an enum with 3 values, but that would break compatibility in 3.x. However we could do this properly in 4.x.
  • I've updated the mechanism for debugging the redraw_requests to support the DEV_ENABLED flag. Now no recompile is necessary on DEV builds, you can place the breakpoints immediately in _changes_changed().

Supported so far

  • Shaders using TIME
  • AnimationTree
  • AnimationTreePlayer
  • Particles
  • CPUParticles
  • Particles2D
  • CPUParticles2D
  • GIProbe

@lawnjelly lawnjelly requested review from a team as code owners October 6, 2021 07:46
servers/visual_server.cpp Outdated Show resolved Hide resolved
servers/visual_server.h Outdated Show resolved Hide resolved
@akien-mga akien-mga added this to the 3.5 milestone Oct 6, 2021
@lawnjelly lawnjelly force-pushed the vital_redraws branch 2 times, most recently from 8bdf3f7 to 0b7f1d5 Compare October 6, 2021 10:52
@lawnjelly lawnjelly requested a review from a team as a code owner October 6, 2021 10:52
@lawnjelly lawnjelly changed the title Add editor vital redraws only option [WIP] Add editor vital redraws only option Oct 6, 2021
@lawnjelly lawnjelly requested a review from a team as a code owner October 6, 2021 12:33
@lawnjelly lawnjelly requested a review from a team as a code owner October 6, 2021 13:50
@lawnjelly lawnjelly changed the title [WIP] Add editor vital redraws only option Add editor vital redraws only option Oct 6, 2021
@lawnjelly lawnjelly force-pushed the vital_redraws branch 2 times, most recently from f7c02aa to b995895 Compare October 6, 2021 15:26
@clayjohn
Copy link
Member

clayjohn commented Oct 6, 2021

I'm really not sure about this. I like the idea of reducing the work the engine does while in the editor, but to me this seems to add significant complexity for uncertain benefit.

When would it make sense for a user to set it to vital updates only? The tradeoff is essentially trading your scene working for extra battery life. So the possible subset of users for this feature are developers who are working from laptops that aren't plugged in and who don't care about their VFX being visible.

Further, the redraw requests only happen when the editor viewport is open, so the battery savings are limited to the periods of time when the user is working in the viewport. I would expect that users would want to see their animations, particles, and shaders while in the viewport.

Further, users will have to turn this setting off if they want to develop particle effects, shader effects, or animations. To me, that seems like a real hindrance.

@lawnjelly
Copy link
Member Author

lawnjelly commented Oct 6, 2021

Well, I'm not using a laptop, but a fanless PC, and for the last 2 and a half years using godot I've had to make sure to manually close scene tabs, and do stuff to turn off shaders to prevent overloading my PC. For people like me, it means we just switch one option and we are sorted. We can program gdscript without worrying about which scenes are open in the background etc.

Then on the rare occasions we need to edit a time shader or something, we just switch it back on. It's actually really useful for people on non-monster PCs.

There's also a whole section of people who are quite eco-minded about power use and don't want to be constantly using CPU cycles for no reason, or want those cycles available for other tasks in the background. Running Godot on an old laptop, or raspberry PI, you can't really do that and run even a browser at the same time. With this it makes all this possible on basic machines.

Further, the redraw requests only happen when the editor viewport is open

In theory yes, in practice no. I'm sitting here with blubits demo project open and it's quite happily spinning away while I'm editing gdscript, or have a 2d window open with nothing visible updating...

@KoBeWi
Copy link
Member

KoBeWi commented Oct 6, 2021

btw there was a proposal about it: godotengine/godot-proposals#942

@clayjohn
Copy link
Member

clayjohn commented Oct 6, 2021

I've had to make sure to manually close scene tabs, and do stuff to turn off shaders to prevent overloading my PC

My comment above was made under the assumption that shaders only force a redraw while they are visible in the viewport and the viewport is visible.

If we could fix it so that the redraw only occurs when the element that needs to be redrawn is visible, would that change the need for this proposal?

@jordo
Copy link
Contributor

jordo commented Oct 6, 2021

I've had to make sure to manually close scene tabs, and do stuff to turn off shaders to prevent overloading my PC. For people like me, it means we just switch one option and we are sorted. We can program gdscript without worrying about which scenes are open in the background etc.

Shouldn't this just be fixed? Why draw scenes in the background in the first place if they aren't visible?

Copy link
Contributor

@jordo jordo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I don't love is the addition of && OS::get_singleton()->_are_low_priority_redraws_enabled() scattered conditionals. Whats the rule here for future additions and checks? How can i tell what VS stuff should be guarded by _are_low_priority_redraws_enabled or not?

So only comment, is that it's kinda scattered around where these checks need to go as it's different for particles, animation tree, tree players, and I'm guessing anything in the future.

Out of curiosity, why choose these specific things to put into _are_low_priority_redraws_enabled whats the threshold criteria, or is it arbitrary?

@lawnjelly
Copy link
Member Author

lawnjelly commented Oct 6, 2021

Yes sure the updating when not selected should be fixed. This was a longstanding problem and I think a PR was done about a year ago which helped prevent it (I forget the details). But it's a multi-surface problem I suspect - anything anywhere that gets updated can request a redraw. And anything calling the VisualServer for most functions also implicitly requests a redraw, including addons etc.

If we could fix it so that the redraw only occurs when the element that needs to be redrawn is visible, would that change the need for this proposal?

No, not really, that's sort of an 'also' problem that is derailing the main point. The issue is that on some machines the editor is difficult to use on a complex project because of the constant unneeded processing, and it's a massive pain. You can't select something in the UI because it's busy updating everything because of a shader that you aren't even interested in seeing. Then some program in the background can't run properly because Godot is hogging all the CPU (while you are maybe out of the room making a cup of tea!). So in practice you end up shutting Godot down all the time because it is too expensive to leave running.

The PR itself is not super complex.

  • Instead of all redraw requests having the same priority, they are stored as low and high
  • The 6 or so major places in the code (animation, particles, shaders) respect the user setting whether to do their processing

So only comment, is that it's kinda scattered around where these checks need to go as it's different for particles, animation tree, tree players, and I'm guessing anything in the future.

I started off just by altering the redraw_requests in the VisualServer. The problem for things that come from scene side, like animation is two fold:

  1. If we just make their redraw requests low priority, it will still use a load of CPU doing scene calculations. (i.e. we will save GPU and a bit of CPU but we are doing a lot of unneeded processing)
  2. The biggie - Any time almost anything touches the VisualServer, it calls a macro which issues a redraw request. So it makes it very difficult to avoid, and the easiest way is to just say 'don't do any processing at all' on the notification.

See visual_server_raster.h: e.g.

#define BIND1(m_name, m_type1) \
	void m_name(m_type1 arg1) { DISPLAY_CHANGED BINDBASE->m_name(arg1); }

// (where DISPLAY_CHANGED causes a redraw_request)

As to which areas have this in, redraw_request has already existing a piece of debugging code for identifying what is causing updates, I made it a little easier to use here. You essentially just run the editor with a scene, and if the spinner is spinning, you put a breakpoint and find the cause. This is how I identified the scene side areas.

This is new, but the old version is roughly similar except you historically had to recompile (but no longer). You can now just put a breakpoint on the if (p_priority) { line, and look at the call stack and you can instantly see what is causing a redraw:

	static void _changes_changed(int p_priority) {
		if (p_priority) {
			;
		}
	}

public:
	// if editor is redrawing when it shouldn't, use a DEV build and put a breakpoint in _changes_changed()
	_FORCE_INLINE_ static void redraw_request(bool p_high_priority = true) {
		int priority = p_high_priority ? 1 : 0;
		changes[priority] += 1;
#ifdef DEV_ENABLED
		_changes_changed(priority);
#endif
	}

There are a few cases in things like process notifications where you could potentially just turn off INTERNAL_PROCESS etc as an alternative. We did this for the Camera a while back. But the logic is a bit messy and the bulk of the benefit can be done simply by wrapping the notification in an OS::get_singleton()->_are_low_priority_redraws_enabled(). Job done, simple as that.

@jordo
Copy link
Contributor

jordo commented Oct 6, 2021

Then some program in the background can't run properly because Godot is hogging all the CPU (while you are maybe out of the room making a cup of tea!). So in practice you end up shutting Godot down all the time because it is too expensive to leave running.

Right, but this is kinda to be expected as the editor itself uses the game engine and will repaint everything, not a graphics stack that will only repaint areas that change and need recomposition. I'm guessing is why any change to VS is going to redraw, which is expected for a game engine renderer.

And isn't the background CPU usage issue covered by platform specifics about whether or not the editor is a foregrounded/focused window or not? The main tick should be throttled to something very low (4-6fps is common) for windows or applications that aren't in the foreground or focused.

You can't select something in the UI because it's busy updating everything because of a shader that you aren't even interested in seeing.

The above seems like an issue of doing some work on the main engine thread, that should have probably be offloaded to a worker?

A side note, is I sometimes use performance in the editor as a general baseline & feeling to how something is going to perform exported.

@lawnjelly lawnjelly requested a review from a team as a code owner January 31, 2022 19:48
@lawnjelly
Copy link
Member Author

If someone turns on vital redraws only, we can probably safely assume he wants as few redraws as possible, so yes.

This is done now, I've added a is_caret_blink_active() function to EditorSettings with the logic for this. It could do with double checking as I'm tired, just to check the EditorSettings will always be available in builds it is used (I think they are wrapped in TOOLS_ENABLED).

doc/classes/OS.xml Outdated Show resolved Hide resolved
core/os/os.h Outdated Show resolved Hide resolved
@lawnjelly lawnjelly force-pushed the vital_redraws branch 2 times, most recently from 75618d9 to 21b554a Compare February 1, 2022 13:21
core/os/os.h Outdated Show resolved Hide resolved
@lawnjelly lawnjelly force-pushed the vital_redraws branch 2 times, most recently from 0a1ec0c to 1eb35cc Compare February 1, 2022 14:58
@lawnjelly lawnjelly changed the title Add editor vital redraws only option [WIP] Add editor vital redraws only option Feb 2, 2022
@lawnjelly
Copy link
Member Author

lawnjelly commented Feb 2, 2022

Just made some small changes:

Summary

I noticed that in the old state, particle systems would not display at all when first loaded (aside from the particle 2D icon). This was occurring because particle systems need to be updated at least a few times in order to emit some particles(!). This was confusing in terms of user experience. I've done a fix for this and the experience is vastly better.

The problem was occurring because particles were not being processed at all in vital updates mode. So I made a slight modification so that a _pending_update flag is set whenever a frame is drawn (due to high priority redraws). This in turn causes particle systems to process in the next redrawn frame. (There is thus actually a delay of one frame before this takes effect, but this isn't a problem in practice, it works very well - we just want to get the particle systems moving a little.)

As a result of this I have unified the function name to is_update_pending() which is a little more descriptive for where it appears in the code and doesn't involve negation logic. The one exception for the descriptiveness is the use in main::iteration but hopefully this is explained enough there with comments.

Note that the update with redraw is not used for the animation players. The reason being that the act of processing the animations calls the VisualServer (to place bones), which in turn triggers the changes_changed macro, and thus a feedback loop of continuous updates would occur. It may be possible to figure a way of getting this working at a later date, but there is no pressing need compared to the particles.

When editor continuous redraws is switched off, the editor only redraws when a redraw_request was issued by an element in the scene. This works well in most situations, but when scenes have dynamic content they will continuously issue redraw_requests.

This can be fine on high power desktops but can be an annoyance on lower power machines.

This PR splits redraw requests into high and low priority requests, defaulting to high priority. Requests due to e.g. shaders using TIME are assigned low priority.

An extra editor setting is used to record the user preference and an extra option is added to the editor spinner menu, to allow the user to select between 3 modes:

* Continuous
* Update all changes
* Update vital changes
@lawnjelly lawnjelly changed the title [WIP] Add editor vital redraws only option Add editor vital redraws only option Feb 2, 2022
@akien-mga akien-mga merged commit 689f59d into godotengine:3.x Feb 4, 2022
@akien-mga
Copy link
Member

Thanks!

@Calinou
Copy link
Member

Calinou commented Feb 13, 2022

I wonder if the vital redraws only option could be automatically enabled when the project is running (and then disabled when the project is stopped). This would fix performance issues when running a project while an animation is being displayed in the editor. There's a FPS limiter when the editor is unfocused right now, but it doesn't always suffice on slower hardware.

To support this, I guess an additional boolean editor setting should be added, such as "Automatic Vital Redraws Only When Running Project".

@lawnjelly
Copy link
Member Author

Yeah we can certainly have a look at this. 👍

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.

None yet

7 participants