AcademySoftwareFoundation / MaterialX

MaterialX is an open standard for the exchange of rich material and look-development content across applications and renderers.
http://www.materialx.org/
Apache License 2.0
1.86k stars 352 forks source link

MaterialXGraphEditor feedback #1207

Closed dgovil closed 1 year ago

dgovil commented 1 year ago

Hi, First of all, really great work on the new graph editor.

I just built off of ec0f81f542add51b1e0043292d198866d08f766b and hit a few issues that I didn't see in the Known Limitations section, so I thought I'd file an issue for them:

  1. Loading a material from the command line doesn't seem to populate the graph. E.g MaterialXGraphEditor --material resources/Materials/Examples/StandardSurface/standard_surface_carpaint.mtlx loads the Material in the view, but no graph nodes are displayed.

    image
  2. Double clicking nodes causes a segfault. I believe it's hitting an unitialized variable, but I haven't gone spelunking with the debugger yet. image

These issues were encountered when running on macOS 13.2, on both x86_64 and arm64, but I think they're issues that aren't platform specific. Otherwise, this is really great work. A very useful tool.

lfl-eholthouser commented 1 year ago

Hi, Thanks for giving the Graph Editor a try. I'll take a look at the material command line argument. Thanks for flagging that!

For the second issue I haven't been able to recreate it on Windows or Linux when double clicking nodes. Is there a specific node you remember trying? I'll test that one as well.

dgovil commented 1 year ago

If I load in resources/Materials/Examples/StandardSurface/standard_surface_carpaint.mtlx for example, and click on the either of the nodes, it'll crash. It doesn't even require double clicking actually, just a single click seems to do it.

image

However, if I put down an outputvector2 node, it doesn't crash when I click on it. Actually, it seems to only crash for nodes from files I load from the examples directory?

dgovil commented 1 year ago

@lfl-eholthouser So the crash happens because in UiNode.h the operator== receives a nullptr, and there's no nullptr check there before the call to GetName.

If you change the body of that method to return lhs != nullptr && rhs != nullptr && lhs->getName() == rhs->getName(); it should resolve the crash.

I believe that the reason it may only be triggering on Mac is down to how different compilers/OS runtimes will handle the undefined behaviour here. It should still be an issue on gcc/msvc on Linux/Windows but may manifest differently depending on what compiler versions you're using.

Anyway, that one liner should fix it up.

Great presentation today at the ASWF meeting!

lfl-eholthouser commented 1 year ago

Thanks for finding that! @dgovil I'll get that fix in.

dgovil commented 1 year ago

Oh one more issue to report. When I run the graph editor it always puts the BasicInteraction.json file, and sometimes makes the imgui.ini files in the current working directory.

It would be great if these weren't created at all, or if they need to be, they should go to https://wiki.archlinux.org/title/XDG_Base_Directory or the macOS/Windows equivalents to keep working directories clean.

lfl-eholthouser commented 1 year ago

Thanks! @dgovil Also, the node click fix you suggested is in now.

meshula commented 1 year ago

imgui.ini files record your layout of panels and windows across sessions. Here's how to redirect it https://github.com/ocornut/imgui/issues/4294

@dgovil on Mac, would you concur on putting it in ~/Library/Application Support/MaterialXGraphEditor/imgui.ini?

dgovil commented 1 year ago

Yeah I'd say either there, or ~/Library/Application Support/MaterialX/GraphEditor/imgui.ini (in case you theoretically want the viewer to have prefs too at some point) I think on Windows that would be %LOCALAPPDATA%/MaterialXGraphEditor/imgui.ini.

Emma, I know you said you don't have constant access to a Mac, so if you add in the code for Linux or Windows locations, I can help out with the Mac side of it. Though fortunately, the code for Linux and Mac should be very similar modulo the exact location.

jstone-lucasfilm commented 1 year ago

@dgovil I believe these issues should be resolved in the latest build, so let us know how things work on your side. We've removed the need for BasicInteraction.json, though I believe imgui.ini is still generated, and that may be worthwhile to note in a new GitHub Issue.

dbsmythe commented 1 year ago

Running the current version of the Graph Editor (pulled and built minutes ago) on MacOS Monterey / M1 MBPro, the "+" and "-" keys aren't doing anything for me to zoom in/out, though mouse scroll-wheel zooming does work (but a bit touchy and hard to control: a tiny amount of wheel movement does a huge amount of zooming). Do the zoom keys work for anyone else on MacOS? (I'd really like it if I could do a variable zoom in/out using control-Rmouse or alt-Rmouse..)

dbsmythe commented 1 year ago

A few suggestions of hopefully easy-to-do things that I think would greatly improve readability of the graphs and Node Property Editor:

lfl-eholthouser commented 1 year ago

Thanks for flagging the issues with zooming on MacOS and for the Graph Editor suggestions. Those are super valuable! In regards to the +/- issue and scroll wheel speed, I just wanted to confirm that these problems occur only with the Graph Editor and the zooming works as expected with MaterialXView?

dbsmythe commented 1 year ago

In MaterialXView on MacOS, the scroll-wheel zooming works more like I'd like, e.g. it's not overly sensitive and it's easy to zoom in or out to some desired level, and zooming with the mouse wheel in the Graph Editor render window similarly works nicely. It's just in the 2D nodegraph window where mousewheel zooming is "too sensitive" The "+" / "-" keys do not have any effect for me in MaterialXViewer.

lfl-eholthouser commented 1 year ago

Ok, great to know. Thanks you!

jstone-lucasfilm commented 1 year ago

Let's close out this original issue in favor of the more recent #1398, and track new ideas for improvements there.