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

Added progress bar when changing type on multiple nodes #31452

Conversation

pouleyKetchoupp
Copy link
Contributor

This change displays a progress bar while doing operations instead of freezing the editor

The progress bar also flushes the message queue, which fixes these errors in cases with lots of nodes:

ERROR: push_call: Message queue out of memory. Try increasing 'message_queue_size_kb' in project settings.
   At: core/message_queue.cpp:56.

Fixes #23853

@Chaosus Chaosus added this to the 3.2 milestone Aug 18, 2019
@Calinou
Copy link
Member

Calinou commented Aug 18, 2019

Looking at the code, it seems a progress bar is displayed as soon as there are at least 2 nodes in the selection. Since it's usually recommended not to display a progress bar for very short actions, should this minimum be increased? Changing the type of 5 nodes likely won't take more than a second on most hardware.

@pouleyKetchoupp pouleyKetchoupp force-pushed the change-node-type-progress-bar branch from 650dd79 to 47e081a Compare August 18, 2019 12:14
@pouleyKetchoupp
Copy link
Contributor Author

Right now, it's displayed as long as more than one node is selected (there are 2 actions for each node). I'll make more tests to display it only when it takes more time to process.

@pouleyKetchoupp pouleyKetchoupp force-pushed the change-node-type-progress-bar branch from 47e081a to 83f89a9 Compare August 18, 2019 12:38
@pouleyKetchoupp
Copy link
Contributor Author

The time it takes to process node replacements also depends on how many nodes are present in the scene, but I've changed it to something more reasonable. Now it will trigger the progress bar for more than 10 nodes (and refresh on every 10th node).

@akien-mga
Copy link
Member

EditorProgress is known to cause some issues and slowdowns, so this needs to be reviewed properly. Haven't done so yet, but just mentioning why it hasn't been merged yet :)

This also seems to handle quite a corner case, so I'm not sure it's worth adding this to UndoRedo.

@pouleyKetchoupp
Copy link
Contributor Author

Alright, thanks for the heads up!

I just wanted to point out that the stuff in UndoRedo might help in the future to solve more cases of operations causing the message queue to be out of memory :)

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Nov 7, 2019
@aaronfranke
Copy link
Member

@pouleyKetchoupp Is this still desired? If so, it needs to be rebased on the latest master branch.

@pouleyKetchoupp
Copy link
Contributor Author

The issue this PR fixes is related to #35653, so for 4.0 it's going to be fixed using a different approach.

I'm closing since it's probably too much of a corner case to be cherry-picked for 3.2.

@pouleyKetchoupp pouleyKetchoupp deleted the change-node-type-progress-bar branch July 1, 2020 08:14
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.

Crash changing type of multiple nodes
5 participants