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

CompareWithCurrentState uses IM_ASSERT_USER_ERROR #5654

Closed
wants to merge 1 commit into from

Conversation

nicolasnoble
Copy link
Contributor

I believe the current code is too harsh, and will fail on recoverable errors. This uses IM_ASSERT_USER_ERROR instead of IM_ASSERT in order to allow the UI in dev mode to yield errors. Also useful when doing scripting like Lua. Tried locally, and most of those errors seem recoverable properly anyway.

I believe the current code is too harsh, and will fail on recoverable errors. This uses `IM_ASSERT_USER_ERROR` instead of `IM_ASSERT` in order to allow the UI in dev mode to yield errors. Also useful when doing scripting like Lua. Tried locally, and most of those errors seem recoverable properly anyway.
@ocornut
Copy link
Owner

ocornut commented Sep 7, 2022

We can head toward that change but I believe for each value we need to ensure safe recovery, which doesn't seem handled by the PR.

In principle the recovery may simply be to assign the value back to saved value, but it needs to be checked carefully for each value. Some of them we silently work yet keep growing a stack during the frame, which may be unnecessary if not confusing. Some of them may not be reset on EndFrame or NewFrame. Would need to check one by one.

Linking to #1651

@nicolasnoble
Copy link
Contributor Author

Right, I wasn't totally sure about this. Thanks for pointing me to the right direction. I'll look at the details.

@ocornut
Copy link
Owner

ocornut commented Feb 3, 2023

FYI we have a basic test for ErrorCheckEndFrameRecover() here:
https://github.com/ocornut/imgui_test_engine/blob/main/imgui_test_suite/imgui_tests_core.cpp#L4504

A second test could be added to fine-test that stack have been unwinded correctly, with perhaps extra sanity check.

I would suggest looking at each stack one by one to ensure the size reset is valid. I'll probably do that eventually as I agree this would be useful.

@ocornut
Copy link
Owner

ocornut commented Sep 24, 2024

Closing this as I am working on a larger revamp of those systems.
See #1651 (comment)

@ocornut ocornut closed this Sep 24, 2024
ocornut added a commit that referenced this pull request Sep 27, 2024
… EndDisabled(), PopTextWrapPos(), PopFocusScope(), PopItemWidth() to use IM_ASSERT_USER_ERROR(). (#1651, #5654)
ocornut added a commit that referenced this pull request Sep 27, 2024
…s to IM_ASSERT_USER_ERROR(). (#1651, #5654)

This commit is not meant to be functional as-is (it will break test engine recovery). This is mostly to reduce/remove noise from upcoming commits.
ocornut added a commit that referenced this pull request Sep 27, 2024
Setup a couple of features to configure them, including ways to display error tooltips instead of assserting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants