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

Dragging a property spams history messages, causing rapid memory usage increases due to editor output panel being filled with text #84499

Closed
XuaTheGrate opened this issue Nov 6, 2023 · 10 comments · Fixed by #84795

Comments

@XuaTheGrate
Copy link

Godot version

v4.1.3.stable.official [f06b6836a]

System information

Godot v4.1.3.stable - Windows 10.0.19044 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2060 SUPER (NVIDIA; 31.0.15.3623) - AMD Ryzen 7 5700X 8-Core Processor (16 Threads)

Issue description

I have a plane mesh with a 2048x2048 PNG texture applied. Changing the albedo colour of the texture results in Godot consuming as much memory as possible until my computer freezes, shortly before Godot crashes.

Steps to reproduce

  1. Create a mesh (CSGMesh3D)
  2. Apply a new standard material
  3. Under Material -> Albedo, change the material's colour.
  4. Rapidly changing the colour results in extreme memory consumption, and won't be cleared until reopening Godot

Minimal reproduction project

N/A

@Calinou
Copy link
Member

Calinou commented Nov 6, 2023

Please upload a minimal reproduction project1 to make this easier to troubleshoot.

Footnotes

  1. A small Godot project which reproduces the issue, with no unnecessary files included. Be sure to not include the .godot folder in the archive (but keep project.godot).

    Drag and drop a ZIP archive to upload it. Do not select another field until the project is done uploading.

    Note for C# users: If your issue is not Mono-specific, please upload a minimal reproduction project written in GDScript or VisualScript. This will make it easier for contributors to reproduce the issue locally as not everyone has a Mono setup available.

@XuaTheGrate
Copy link
Author

Here is the project: GodotBug.zip

  • Open godot_bug.tscn
  • Select the CSGMesh3D
  • Under Inspector, select and expand Material
  • Go to Albedo -> Color, and change the color of the texture.
  • See increased memory usage, more visible if you rapidly change the color of the texture.

@TheSofox
Copy link
Contributor

TheSofox commented Nov 7, 2023

I was able to reproduce this issue with a Sprite in a simple 2D scene by rapidly changing the colour of the "Modulate" property. I was easily able to get the RAM usage to jump by a Megabyte every few seconds.

@TheSofox
Copy link
Contributor

TheSofox commented Nov 8, 2023

I was able to reproduce this issue in the most unlikely way possible: Going to the Spinner of the Z-Index of a sprite, and just holding down on the arrow key so it spins through various values. From this it seems that this problem is caused by any rapidly changing value in the Inspector.

@TheSofox
Copy link
Contributor

TheSofox commented Nov 8, 2023

Think I've found the issue. It's the Output Log at the bottom of the Editor. Having it spammed with "Set albedo_color" or "Set Z-Index" assuredly takes its toll on RAM usage, especially since the Output log doesn't get cleared unless the editor is shut down (even changing scene doesn't clear it).

The weird thing is that looking at the code, it does seem that some functionality had been laid down to handle getting a series of repeating log messages. ( in editor\editor_log.cpp ).

@Calinou
Copy link
Member

Calinou commented Nov 9, 2023

Message collapsing is disabled by default. You need to enable it by clicking the "3 horizontal lines" icon on the right of the Output panel:

image

Here's an example of it in action, both with print() and editor messages:

image

I'd prefer if it was enabled by default, but this can likely only be changed in 4.3 since we're in feature freeze.

We could also reduce the number of editor history messages being printed in the first place, but I recall this being nontrivial when you drag a property (so that only one message is printed at the end of dragging).

Lastly, we could get rid of these messages entirely as the History dock now features them in a similar fashion.

@Zireael07
Copy link
Contributor

Zireael07 commented Nov 9, 2023

Lastly, we could get rid of these messages entirely as the History dock now features them in a similar fashion.

Please do this.

@TheSofox
Copy link
Contributor

TheSofox commented Nov 9, 2023

Thanks @Calinou, I did more testing with the collapsing feature and it didn't use up extra RAM as I rapidly changed a feature.
I then disabled collapsing, and RAM instantly shot up a few MB to display all the messages individually.
I then re-enabled collapsing, and annoyingly while the messages collapsed, the RAM usage didn't go down.

@Calinou Calinou changed the title Changing texture albedo consumes all memory possible Dragging a property spams history messages, causing rapid memory usage increases due to editor output panel being filled with text Nov 10, 2023
@TheSofox
Copy link
Contributor

So here are possible solutions:

  1. Remove Output Log entirely
  2. Enable Collapse setting by default
  3. Optimise RichText so it takes up less memory
  4. Find some way to restore RAM when Output Log is collapsed or cleared
  5. Block any Undo Message/Property Change from being displayed in the Output Log. If it's in the History panel, don't put it in the Output panel
  6. Allow the Property Change messages to be displayed, but use have a bunch of rapid changes of the same property be represented by a single message. This is the same system Undo and History have.
  7. Put a hard limit on the amount of lines in the Output Log, possibly editable in a settings menu

There's a lot to consider, but after giving it some thought I realised that (6) was the most straightforward and easiest change to implement right now to solve this specific issue. Identical changes made less than 800 milliseconds apart only give a single output message. Previously, messing around with the colour picker could give hundreds of messages, now it will only give a handful. Since it reuses the same system that UndoRedo and History log uses, there's both minimal code change and it's consistent.

My pull request is incoming.

@TheSofox
Copy link
Contributor

Pull request made. I suggest that (pending review) this pull request is used to close this specific issue. Further discussion on what to do about the Output Log should continue in other places (such as the other issues that have been linked to this one)

@akien-mga akien-mga added this to the 4.2 milestone Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants