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 Native File Browser for Graph Editor #1256

Conversation

dgovil
Copy link
Contributor

@dgovil dgovil commented Feb 24, 2023

This PR adds support for using a platform native file browser on macOS to the MaterialXGraphEditor.

It introduces a new class that matches the Imgui File Dialog API, though limited to just the calls used by the GraphEditor.
Being able to use the native browser allows for quicker navigation to locations the user may have saved, and allows handling of some cases like cloud synced files.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dgovil / name: Dhruv Govil (95a0e84)

@dgovil
Copy link
Contributor Author

dgovil commented Feb 24, 2023

I went through the CLA registration process and got a confirmation. Not sure if there's a lag time between that and the test updating here

Edit: I think it's because I submitted the commit from my corporate email. Which I've also added to my account, and signed a secondary CLA for, but it looks to not like that. Submitted a JIRA ticket here: https://jira.linuxfoundation.org/plugins/servlet/desk/portal/4/SUPPORT-16422

In the meantime, I've updated the PR with my personal email to unblock it

Copy link
Contributor

@kwokcb kwokcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to get rid of the built in browser on desktop. I "think" it would still be useful as a runtime option however as believe that all UI drawn using ImGui constructs so maintains consistency with the rest of the UI which can have customizable attributes set, and maybe also is more cross-platform (non-desktop) ?

source/MaterialXGraphEditor/Graph.h Outdated Show resolved Hide resolved
@jstone-lucasfilm
Copy link
Member

In a Slack discussion with @dgovil, he suggested the idea of importing the existing cross-platform file browser from NanoGUI into MaterialXGraphEditor, effectively replacing the dependency of the graph editor on ImGuiFileBrowser. This sounds like a good approach to me, and further in the future we might consider moving the imported file browser code from MaterialXGraphEditor into MaterialXRender, making it available for other example applications to use. This future step would require either raising the minimum supported platform for MaterialXRender to C++17, or replacing the std::filesystem references in the file browser code with equivalent functions from MaterialXFormat.

This PR copies the file browser handling code from NanoGUI, and adds an adapter to be able to use it with Imgui
@dgovil dgovil changed the title Added macOS Native File Browser for Graph Editor Added Native File Browser for Graph Editor Mar 27, 2023
@dgovil
Copy link
Contributor Author

dgovil commented Mar 27, 2023

@jstone-lucasfilm and @kwokcb , I've updated this PR to include the code from the NanoGUI end so in theory it should work on macOS, Windows and Linux. I've only tested this PR on macOS since I don't have a Linux or Windows machine handy.

Would someone else be able to test+modify the PR for those platforms? I think I captured all the includes and CMake configurations that might be needed, but I am not sure of course.

Thanks!

@kwokcb
Copy link
Contributor

kwokcb commented Mar 28, 2023

Hi @dgovil,
I've checked out Windows. Looks good from my testing.
I don't have access to modify your branch but here's the diff to fix windows builds and clear the file filter properly.
patch.diff.txt

Sorry I don't have Linux. Add in @lfl-eholthouser, to see if she has it, and for review.

@dgovil
Copy link
Contributor Author

dgovil commented Mar 28, 2023

Awesome, thanks @kwokcb . I just applied your patch (just a note, I think your diff was inverted but easy enough to flip)

@kwokcb
Copy link
Contributor

kwokcb commented Mar 28, 2023

Sorry about that. Must have got args reversed in git diff. :).

@lfl-eholthouser
Copy link
Contributor

lfl-eholthouser commented Mar 30, 2023

Hi @dgovil, I've checked out Windows. Looks good from my testing. I don't have access to modify your branch but here's the diff to fix windows builds and clear the file filter properly. patch.diff.txt

Sorry I don't have Linux. Add in @lfl-eholthouser, to see if she has it, and for review.

Hi @dgovil I tried it out and its working for me on Linux! It looks great! I'm getting a Linux error caused by IM_ifgreater_vector3_genglsl that's affecting the rendering so nothing shows up in the render view. I'm pretty sure this is related to a different change so I'm going to keep investigating that, but for some reason even just adding new folder and the changes to the Graph.h are causing it.

@dgovil
Copy link
Contributor Author

dgovil commented Mar 30, 2023

Thanks for confirming it works!

That error does sound like it's coming from elsewhere unless some kind of preprocessor define caused a conflict?

Though I'd be surprised since the code is largely from nanogui so I'd expect to see the same issue in materialxview

Hopefully it's easy enough to track down

This changelist merges the new FileDialog class into the main source folder for the Graph Editor, creating a slightly simpler structure.  Additionally, it removes a dependency of FileDialog.h on UiNode.h, as a step towards moving this code to MaterialXFormat in the future.
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
@kwokcb
Copy link
Contributor

kwokcb commented Apr 3, 2023

The only code that I know which may overlap should be the code for handling focus. i.e. @lfl-eholthouser I assume you added in the code to check focus since the older dialogs being drawn using OpenGL. I don't think they would affect anything but that's all I can think of. Would be good to clean up this code afterwards though.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this proposal together, @dgovil, and this looks good to me!

@jstone-lucasfilm jstone-lucasfilm merged commit 612cb94 into AcademySoftwareFoundation:main Apr 3, 2023
12 checks passed
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
…#1256)

This PR adds support for using a platform native file browser to the MaterialXGraphEditor.
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.

None yet

4 participants