Open pthom opened 1 month ago
Side note: I did add a related patch for the StackLayout feature (ImGui::Begin_Horizontal + ImGui::Begin_Vertical), because reusing the same ID would lead to an infinite loop (and Python users would not be able to guess the reason for this infinite loop): see https://github.com/pthom/imgui/commit/8b45d6bcea37311f4fa29388b3958e89393b2650
This implementation is unfortunately extremely CPU costly, to a point where it is going to be unacceptable for performance. Even if you used a better data structure for it, it would likely be too much. On a complex UI this is likely to cost more than the entirety of the rest of Dear ImGui. We quite rarely perform non-O(1) operations on standard widgets.
My strategy for this would be to perform a compare based on a shared field that's normally set once a frame, e.g. g.HoveredId
, so it would become a lightweight and O(1) check, with the limitation that it would trigger during interaction (e.g. hover), but considering the alternative is redhibitory it seems like a good compromise.
I don't necessary agree that an assert is ideal, but we would turn that detection into an overlay warning with a button to break into either items. We would need to use there is a way (e.g. via ItemFlags) to allow it as I am convinced there are situation where it would be useful.
Hello again Omar,
Many thanks for your quick answer!
This answer is only related to the CPU cost part : the question whether this is a good idea, or whether this is a good way to implement this is still opened.
I did measure the CPU usage of my implementation.
For this, I used a simple benchmark utility, see this commit: https://github.com/pthom/imgui/commit/839d331d7afbd4bf9f9c829a14342c5d0684af24
And I tested this with quite a lot of widgets. See video below:
https://github.com/ocornut/imgui/assets/7694091/c5b3299e-5c35-4737-bd70-1f498b3e7a82
And the results are (for 1 minutes usage of an application with lots of varied widgets)
Function |Nb calls|Total time|Av. time|Deviation|
----------------------+--------+----------+--------+---------+
GetId(const char*) | 821501| 126.161ms| 0.154us| 2.275us|
GetID_AssertUniquePart| 260980| 19.860ms| 0.076us| 0.207us|
GetId(int) | 91992| 3.090ms| 0.034us| 0.097us|
GetId(void*) | 10523| 0.629ms| 0.060us| 0.091us|
PS: I wrote this benchmark utility in a previous life. It is quite easy to setup, which is why I chose it. I suspect another benchmarking tool would lead to similar results.
And I tested this with quite a lot of widgets. See video below:
Looks like quite a few widgets, but separated into different tabs and not that many widgets each frame. Your implementation searches linearly through a vector for each id it has to check while the same vector grows linearly as well. That's quadratic runtime and can escalate pretty quickly.
If the deviation in your benchmark table is what I think it is (difference between minimum and maximum time per call), then GetId(const char*)
already grows to take almost 15x as long as the average call. And your average call already includes the id check, so even worse compared to the unmodified version.
It’s indeed a small amount of widgets. Calling 1-10k/frame would be a good base but i don’t need to see benchmarks to know it’s going to be too expensive. The entire codebase is designed to avoid this sorts of scan as much as possible.
It’s indeed a small amount of widgets. Calling 1-10k/frame would be a good base but i don’t need to see benchmarks to know it’s going to be too expensive. The entire codebase is designed to avoid this sorts of scan as much as possible.
I see your point. I didn't mean to come across as overconfident, and I apologize if I did.
This issue was indeed just a suggestion, and potential food for thought for a possible implementation. If this issue is deemed useful, a totally different implementation could be used, of course.
Regarding the cache, it's quite small and could be limited to a max size (e.g., 100 entries, around 400 bytes). This would fit easily in the L1 cache, making a linear scan relatively fast. This revised implementation is an attempt in this direction.
I provided a solution that would be near free and react on hover, it seems like a good enough tradeoff ?
My strategy for this would be to perform a compare based on a shared field that's normally set once a frame, e.g. g.HoveredId, so it would become a lightweight and O(1) check, with the limitation that it would trigger during interaction (e.g. hover). we would turn that detection into an overlay warning with a button to break into either items. We would need to use there is a way (e.g. via ItemFlags) to allow it as I am convinced there are situation where it would be useful.
I'm not sure I understood how to implement your proposed solution.
perform a compare based on a shared field that's normally set once a frame,
This is the part I do not understand
we would turn that detection into an overlay warning with a button to break into either items
IMHO we would need to keep track of the already registered IDs somewhere. Am I wrong here? In that case, we could limit the loop to only when the item is hovered.
But actually I think you had another idea which I did not understand.
Anyhow, there is no emergency.
Cheers
PS: I'm quite busy on another project at the moment. I might contact you by email in the next days to tell you about it. I would like your feedback (if you have time).
Additional info (if using the current version of the patch): there is an additional related commit that is needed https://github.com/pthom/imgui/commit/97622b0eb46eded650133be311a1261fce3b1672
After running for quite a while with this patch, the only places that do reuse the Id are TempInputScalar and TempInputText. This additional commit handles this.
Version/Branch of Dear ImGui: Version 1.90.7
Bonjour Omar,
There is a common user issue which is that developers may reuse the same ID, and will not understand why their widget is not responsive.
Example
I did want to add an assert for Python users of Dear ImGui bundle, as they might not be able to understand the reason or the failure.
So I added a patch like the one you can see here: https://github.com/pthom/imgui/commit/ea95177b494c3f7e51bcbdbab24f4f8a9103a0f3
With this patch, the example code would throw an assert for each reused ID.
The patch adds this method:
The principle is to use this method in some rare and carefuly selected locations (ButtonEx, ArrowButtonEx, Checkbox, RadioButton , DragScalar, SliderScalar, Drag, InputTextEx), i.e. the most commonly used widgets, and to warn the user if they are using an existing ID.
The implementation is simple: just remember the already pushed IDs, and raise an assert if the user is raising the same ID in the same frame. This concerns only a very limited set of the IDs (i.e. those for the aforementioned widgets).
Would you be interested in such a change? If so, I can push a PR. I know that my current implementation can be improved and should not use a static variable (instead, perhaps rely on a member of ImGuiWindow)
I know that this change could have some implications in a lot of user code. But I would think that it is for the best because they would be warned that they were doing something bad.
As an encouraging hint, I tested it successfully in all of the numerous demonstrations of Dear ImGui Bundle, and it worked correctly
Feel free to close if you do not feel that this is useful.
Note: this feature could be protected by a compilation flag, in order to avoid potential regressions in user code.
Cheers!