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

Text editor undo/redo representation is very inefficient and slow, and can only store 1024 keypresses by default, also causes bugs #101204

Open
wareya opened this issue Jan 7, 2025 · 3 comments

Comments

@wareya
Copy link
Contributor

wareya commented Jan 7, 2025

Tested versions

4.4dev7 but also many other versions of 4.x like 4.3 betas

System information

Godot v4.3.beta2 - Windows 10.0.19045 - GLES3 (Compatibility) - AMD Radeon RX 6800 (Advanced Micro Devices, Inc.; 32.0.12033.1030) - AMD Ryzen 5 7600X 6-Core Processor (12 Threads)

Issue description

When multiple keypress actions are combined into a single logical "operation" in the undo/redo history, rather than being combined into a single undo/redo history item, they're associated together by adding flags to the first and last sub-operations that make up that one undo/redo operation.

This means that single characters have their own whole entire linked list item and that undoing large actions is slow. (In my video below you can see a visible delay when undoing one particularly large typing operation. That is not a recording artifact; it is how slow the undo actually is.) This also makes it easier for old inputs to fall off the back of the undo/redo stack, which has a length limit of 1024 by default, and when this happens, buggy stuff happens. For example, redoing after undoing doesn't undo the whole logical operation but rather only a single sub-operation. This is because the oldest accessible sub-operation lacks the "first sub-operation in complex operation" flag.

Consecutive sub-operations of the same type should be combined into a single undo/redo list entry as they are created (and not just when the complex meta-operation is finished/committed).

Ideally, the system of matching flags for detecting complex operations should be removed entirely or reworked into something else (like a 'multi-operation' operation type, maybe); it is just a bad idea and makes it too easy for undo/redo stack manipulations (e.g. removing the oldest operation) to cause bugs.

Steps to reproduce

Write a lot of text into the text editor (at least 1024 characters), then try undoing it all. Example:

Godot_v4.3-beta2_win64_2025-01-06_19-59-23.mp4

Minimal reproduction project (MRP)

N/A, editor bug

@wareya wareya changed the title Text editor undo/redo representation is very inefficient and can only store 1024 keypresses by default, also causes bugs Text editor undo/redo representation is very inefficient and slow, and can only store 1024 keypresses by default, also causes bugs Jan 7, 2025
@kitbdev
Copy link
Contributor

kitbdev commented Jan 7, 2025

There is a project setting to increase this, but the undo system does need to be reworked.

They can't be merged as they are created since it doesn't work with multiple carets and it caused issues with merging unrelated operations with the action system (that's how it used to work). They need to be merged afterward, which is more complicated. I think that the 'action' system can be repurposed to do the merging instead of messing with complex operations like it currently does.

The code removed in #85325 shows how the system can already support multiple characters at a time.

@wareya
Copy link
Contributor Author

wareya commented Jan 7, 2025

Can't they be merged as they're created (rather than when the complex action is committed), but only if the editor's in the state that's currently used to chain complex actions together, and also only as long as you require a single action to be of a single type? i.e. no merging NEWCHAR ; NEWCARET ; NEWCHAR, only NEWCHAR ; NEWCHAR. Like if I just type "asdf" I can expect that to turn into one event before I'm done even if it's going into like seven cursors or something.

@wareya
Copy link
Contributor Author

wareya commented Jan 8, 2025

Made #101261 to demonstrate what I mean by "merged as they're created". In my idea, the merging happens in the operation management system rather than in the operation itself. Not sure I got it 100% right (it passes tests and feels right when using it in a test project) but it's just a proof of concept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants