ed::EditorContext::LoadSettings() - inverted "m_VisibleRect" test

[crolando]

im on the develop branch.

in imgui_node_editor.cpp..
in ed::EditorContext::LoadSettings() ...

line 2045 there is this if statement, that guards assigning the deserialized m_Settings to the m_NavigateAction, specifically the scroll and zoom:

    if (ImRect_IsEmpty(m_Settings.m_VisibleRect))
    {
        m_NavigateAction.m_Scroll = m_Settings.m_ViewScroll;
        m_NavigateAction.m_Zoom   = m_Settings.m_ViewZoom;
    }
    else
    {
        m_NavigateAction.NavigateTo(m_Settings.m_VisibleRect, NavigateAction::ZoomMode::Exact, 0.0f);
    }

I think this test is inverted. That is, that it should read:
if (!ImRect_IsEmpty(m_Settings.m_VisibleRect))

I found that when I invert this test, then my scroll and zoom deserialization works correctly.

The json that is saved out during normal serialization has a valid rectangle like this:
"visible_rect":{"max":{"x":1623.708251953125,"y":-1014.2291259765625},"min":{"x":718.125244140625,"y":-1469.4290771484375}}

So the current code looks at that valid rectangle, and ImRect_IsEmpty correctly returns "false" (because it's not empty), and this skips the scroll and zoom assignment. What happens next is the "else" clause is used, which recomputes and uses new zoom and scroll values. So this "throws out" the deseralized zoom and scroll from the json file, and gives you a default-ish zoom and scroll.

[thedmd]

Idea behind this change is to save visible area and restore view using this rectangle. This way when you open editor in window with different size, you will see same content as when closing. Does it work differently for you?

[crolando]

Does it work differently for you?

It is broken.

When settings are saved, the view rectangle is such that
min.x < max.x, and min.y < max.y

This is serialized correctly on close, and parsed (in ed::EditorContext::LoadSettings() ) correctly on open.

All valid deseralized settings will fail the if (ImRect_IsEmpty(m_Settings.m_VisibleRect)) test. Because it fails, the zoom factor that is present in the json file is thrown out, and set to zero by: m_NavigateAction.NavigateTo(m_Settings.m_VisibleRect, NavigateAction::ZoomMode::Exact, 0.0f);

I found that if I inverted the test, this causes the original rectangle AND zoom to be used, and then the closed and re-opened views are the same.

Is the problem that the zoom is set to zero in m_NavigateAction.NavigateTo(m_Settings.m_VisibleRect, NavigateAction::ZoomMode::Exact, 0.0f); ?

Is the else clause the one that is triggered when:

This way when you open editor in window with different size
?

Because, the else clause is always called for me, which makes the view different every time between close and open with the same window size.

[thedmd]

@crolando I'm trying to tackle this problem. Let's investigate in more details.

m_Scroll and m_Zoom are internal state of the NavigationAction, they were serialized but they lack of necessary information to correctly restore view unless editor is open in the identical size as it closed.

New code save m_VisibleRect, which is canvas area visible in editor while saving. When rect is saved, zoom and scroll values are not. This finish migration and from this point only rect is used in editor state.

When m_VisibleRect is not present (ImRect_IsEmpty() returns true), there is a fallback to old method.
Otherwise NavigateAction is told to make canvas show area pointed by m_VisibleRect, zooming to match editor size. If in new session editor is larger, you should see same content as in small editor.
Do you by any chance expect editor to not touch zoom value at all and only center on the last visible area?

Last passed argument '0.0f' is not zoom, but animation duration - telling action to navigate instantly.

I argue that the code is correct. Am I missing something?

    if (ImRect_IsEmpty(m_Settings.m_VisibleRect))
    {
        m_NavigateAction.m_Scroll = m_Settings.m_ViewScroll;
        m_NavigateAction.m_Zoom   = m_Settings.m_ViewZoom;
    }
    else
    {
        m_NavigateAction.NavigateTo(m_Settings.m_VisibleRect, NavigateAction::ZoomMode::Exact, 0.0f);
    }

[crolando]

Thanks for the write-up - I need see time before I can get back to it.

[thedmd]

After what I said, I found other unrelated issue that made NavigateTo() to not work on first frame.
This may be an issue you actually experiencing. Let me fix that I will hand over code to you for confirmation.

[thedmd]

@crolando I pushed fix that may help. Can you confirm it on your side?

[mtrebi]

Hey, first time trying to use the library and I am experiencing this very same issue with the code from the simple_example.cpp.
Changing the condition as @crolando suggested seems to fix the issue.

Great work btw!

[thedmd]

Hey,

Do you by any change have a repro of this issue?

Thanks!

[crolando]

@crolando I pushed fix that may help. Can you confirm it on your side?
I need see time before I can get back to it.

I'm back!

Thank you for taking the time to debug and fix this. Everything is working on my end now on both master and develop branch. I believe you fixed it with the "first frame" bug fix to NavigateTo().

@thedmd I am willing to close this. Should we wait for @mtrebi 's repro?

[thedmd]

I'm fine with closing this. Thank you for confirming issue is solved.