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

[3.x] Editor 3D view mesh stats #88207

Merged
merged 1 commit into from
Apr 21, 2024
Merged

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Feb 11, 2024

Similar to information window, add a small optional window to display face count and other stats.

Implements godotengine/godot-proposals#248

stats4

Vertex Format

Is supported internally but display of the line is commented out for now in the source code so we can leave to a follow up PR. See discussion in later comments.

Notes

  • Switched on an off via View Selected Info in the 3D window (defaults to off).
  • This is pretty simple, but is super useful for performance optimizing scenes.
  • The standard information window is useful, but it doesn't allow us to easily examine individual meshes.
  • Supports MeshInstance and MultiMeshInstance so far.
  • Information is hidden unless relevant. If one mesh is selected, that line is hidden, etc.
  • Counts are added when multiple objects are selected.
  • Vertex format is ORed when multiple objects are selected. This was best compromise I could think of.
  • I did try various options for displaying vertex format - all caps, capital first letter only etc. This version looked best so far but happy to hear ideas.
  • Drawcalls can be inferred from number of surfaces, material swaps might be nice for a followup.
  • Index count can currently be inferred from tri count, but might still be useful for non-tri based geometry (could arguably be hidden though)
  • Index count / Vertex count determines vertex sharing measure. This could be displayed also, but it's usually obvious from the figures.

Discussion

One thing that could be worth improving is that it currently doesn't have a label to tell you this box is for the selected objects. I'm hoping it's kind of obvious, once you have it switched on. Colors could be changed to distinguish it from the Information label. Position wise I don't know if anything else uses the lower left 😀 , I don't use anything else regularly that does.

Also I'm happy to hear other suggestions where to put this info in the editor, am happy to change. This seemed to work pretty well so far though.

@rick551a
Copy link

In blender, turning on option 'scene statistics' shows these stats on the status bar, alongside the blender version.
I see godot has alot of prime unused screen real estate on the 'output bar' alongside the version number.
I wonder could a similar 'scene statistics' option allow these stats to be displayed on the output bar?
Just a thought?

@fire
Copy link
Member

fire commented Feb 11, 2024

If you're able can you also make a similar meshs stats pr for master. It's not necessary, but it would make my day.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 11, 2024

If you have to list the meaning of every shortened word there's a bit of an usability issue. It's not exactly clear what those mean and it's worse for other languages. Perhaps it would be nice to show a tooltip on hover, listing the abbreviations and what they stand for.

@Mickeon Mickeon requested a review from a team February 11, 2024 21:37
@lawnjelly
Copy link
Member Author

lawnjelly commented Feb 12, 2024

If you have to list the meaning of every shortened word there's a bit of an usability issue. It's not exactly clear what those mean and it's worse for other languages. Perhaps it would be nice to show a tooltip on hover, listing the abbreviations and what they stand for.

Yup this is very valid. Another idea might be to replace the shortened words with icons, and then have a tooltip (translated into local language). I'm not very good at creating icons / editor UI.

We could simply turn off the display of vertex format in this PR and leave it to a followup, as the vertex format seems the most "bike-sheddy", and may benefit from a more UI focused contributor (providing the internal mechanisms to get the format is preserved from this PR).

UPDATE: I've commented out so removed the vertex format line for now, either for follow up PR or if we can decide best how to do this (so to keep this PR in an "approvable" state).

@lawnjelly
Copy link
Member Author

I had a little time today and I managed to get it displaying the format with icons with tooltips. This is probably more sensible than text, although I discovered I have zero god given artistic or technical talent at creating SVG icons so would need someone else to create these.

format_icons

The vertex format icons still need a bit more polishing up so I'm content to leave displaying vertex format to a follow up PR.

@Calinou
Copy link
Member

Calinou commented Feb 17, 2024

The numbers displayed would benefit from thousands separators, but I think we should add a String method for that as it would also be useful in a lot of places in the editor. Making it i18n-friendly without increasing binary size a lot (due to ICU data) will be challenging though, unless we stick to supporting only a handful of languages.

Edit: Pull request opened for 4.x: #89424

@lawnjelly
Copy link
Member Author

The numbers displayed would benefit from thousands separators, but I think we should add a String method for that as it would also be useful in a lot of places in the editor. Making it i18n-friendly without increasing binary size a lot (due to ICU data) will be challenging though, unless we stick to supporting only a handful of languages.

Absolutely, but there is a danger of feature creep here. It may be better suited to an enhancement PR for general use in the engine - requiring bikeshedding etc independently due to the different internatial standards, and as you say there are several user interface areas which could potentially benefit.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased against 3.x 60ff43b), it works as expected.

However, when moving a node with View Selected Info enabled, the Translating: ... text will appear below the View Selected Info panel:

image

I suggest moving this text to the right when View Selected Info is enabled.

@lawnjelly
Copy link
Member Author

I suggest moving this text to the right when View Selected Info is enabled.

Ah good point 👍 , I did suspect there might be some GUI elements that might overlap but it was kind of hard to figure out. I'll see if I can move one or the other.

We have some space to use a more descriptive label, so I'd suggest this: View Selected Mesh Stats

Good idea.

@lawnjelly
Copy link
Member Author

lawnjelly commented Mar 1, 2024

The text for the messages (like "translating" etc) turns out to be drawn directly to a surface for the whole view as far as I can see, so to move them I'd either have to write some code for their placement, or put them in a Label inside e.g. a VBoxContainer or something.

However I realised there was a much simpler way to deal with this, simply by hiding the selected info when the message is active. This seems to work fine (there's a slight delay after hiding the message before the selected info appears again, but it looks fine). So I've pushed with this latest version.

This was a very simple change (spatial_editor_plugin.cpp, line 2777), effectively:

show_selected_info &&= (message_time <= 0);

@lawnjelly lawnjelly modified the milestones: 3.x, 3.6 Mar 26, 2024
@lawnjelly lawnjelly force-pushed the view_mesh_stats branch 2 times, most recently from f383430 to 562c12c Compare April 20, 2024 09:05
@Calinou
Copy link
Member

Calinou commented Apr 20, 2024

However I realised there was a much simpler way to deal with this, simply by hiding the selected info when the message is active. This seems to work fine (there's a slight delay after hiding the message before the selected info appears again, but it looks fine). So I've pushed with this latest version.

I've tested this and it looks a bit strange, since it takes several seconds for the mesh stats to appear again:

simplescreenrecorder-2024-04-20_21.58.34.mp4

Could the delay be shortened a bit, or could we move the mesh stats box upwards (regardless of whether there's actually a message)? It might look a bit strange, but I think this is better than hiding information.

Similar to information window, add a small optional window to display face count and other stats.
@lawnjelly
Copy link
Member Author

lawnjelly commented Apr 21, 2024

There's now no delay.

The delay was caused by the message timing code which has been slightly adjusted to add a special case for NULLing out the message (which happened when you stopped moving an object).

The previous version was actually bugged for two reasons:

  1. There was an extra unneeded update occurring after setting the message to "" (after the timeout).
  2. The timer was decrementing on the same frame as the message was set (this just meant the timings were previously slightly out by a physics tick, which is unlikely to have been noticed).

Fixing this meant the message_time was set to 0 immediately, and thus the mesh stats appear as soon as you stop moving the object.

		if (message_time > 0) {
			if (message != last_message) {
				surface->update();
				last_message = message;

				// If there is now no message,
				// disable the timing counter.
				if (message == "") {
					message_time = 0;
				}
			} else {
				message_time -= get_physics_process_delta_time();
				if (message_time < 0) {
					surface->update();
				}
			}
		}

The reason I'm trying to avoid moving the text (rather than relying on timing) is two fold:

  1. When the 3D window is small, there's a danger of the moved text overlapping other GUI items (e.g. the mouse speed scaling bar).
  2. The existing message display code looks rather hacked in and is placed manually, and doesn't use a container. The existing code would probably need to be made more sensible prior to moving it (or moving the view mesh stats) via e.g. a vertical split container.

@lawnjelly lawnjelly merged commit 561a8ea into godotengine:3.x Apr 21, 2024
14 checks passed
@lawnjelly lawnjelly deleted the view_mesh_stats branch April 21, 2024 18:32
@lawnjelly
Copy link
Member Author

Thanks!

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.

5 participants