Feature/rm stylized node display#6967
Conversation
|
Reminder to review this please @ericmehl |
ericmehl
left a comment
ericmehl
left a comment
There was a problem hiding this comment.
Thanks a lot @IRONIC3D for contributing this! Those stylized looks nodes are much more reasonably sized with this. I apologize for letting the Windows build description fall behind, looks like you had to jump through some hoops to get this working.
The code for your original goal of reducing the nodules on the stylized shader nodes looks good. I left some comments on the build instructions that hopefully will simplify that.
If you want to get the nodule commit merged before going through the README changes, it might be easiest to just create a new branch, cherry pick that commit and submit it as a new PR for just the nodule changes.
| ../gaffer-build/bin/gaffer | ||
| ``` | ||
|
|
||
| ### Build process on Windows 11### |
There was a problem hiding this comment.
This needs a space between the 11 and the trailing # signs. Otherwise it will display the actual hash symbols rather than interpret it as a formatting code.
|
|
||
| ```bash | ||
| # Must be executed inside Git Bash to interpret the .sh file | ||
| ./config/installDependencies.sh ../gaffer-build |
There was a problem hiding this comment.
I think the use of installDependencies.sh (also mentioned above in the main build instructions) is out of date now. Even easier than using git bash is to run .github/workflows/main/installDependencies.py. On Linux you can run that script directly, on Windows you need to do python .github/workflows/main/installDependencies.py (Windows doesn't recognize Python scripts as being executable).
Might be worth putting this into the shared build steps since it's now cross-platform?
| ``` | ||
| Once the dependencies finish unpacking into your gaffer-build directory, close Git Bash. | ||
|
|
||
| #### 2- Install Scons |
There was a problem hiding this comment.
I'm kind of inclined to let this step be covered by the Main build requirements section above, where scons is mentioned. I don't think scons is installed by default on most Linux distributions, so maybe it's worth adding a note to the shared build steps that it can be installed using pip install scons?
|
|
||
| ```powershell | ||
| :: 1. Navigate to your local clone directory | ||
| cd path\to\your\gaffer |
There was a problem hiding this comment.
I'm pretty sure this shouldn't be needed since pip will install scons for the user / system-wide Python interpreter, independent of the current directory.
| pip install scons | ||
| ``` | ||
|
|
||
| #### 3- Add existing 7-Zip to your System PATH |
There was a problem hiding this comment.
Good catch! This is definitely missing from the instructions and could trip up someone using the installDependencies.py script. I think it might be helpful to add it to the Main build requirements a bit further up in this file.
I'm wary though of including instructions for adding it to the system PATH. That requires administrator privileges that I'm guessing users in a lot of studios won't be able to elevate to.
I think the 7zip installer should handle adding itself to PATH, did you find that not to be the case?
| - Click OK on all windows to save. | ||
| - Restart your terminal and run the script again. | ||
|
|
||
| #### 4- Force Python to use UTF-8 encoding in this PowerShell window |
There was a problem hiding this comment.
Good catch here too, this is needed and unique to Windows. Similar to above, I'm a little reluctant to add it to the system variables. I assume it requires elevation to admin, or the Powershell session itself needs to be admin? It would also have side effects for other Python sessions that may cause issues.
In Powershell, you can do $env:PYTHONUTF8=1 to set it for just the terminal session you're in and not have to worry about it affecting other uses of Python.
|
|
||
| Before you can build you need to setup a few things because of the difference between linux and windows. Windows requires a little bit more hand holding. Make sure you do these steps below for a successful and fluid build. | ||
|
|
||
| ***1. Inkscape Errors:*** Gaffer utilizes Inkscape purely during the compilation phase to automatically convert all of the UI icon assets (.svg vector files) into .png raster graphics for the application layouts. But the problem is that even though Inkscape might be in the environment path scons will not find it. So we have to force it in the command line. The line to add at the end of the build command is `INKSCAPE="C:\Program Files\Inkscape\bin\inkscape.exe" --config=force` |
There was a problem hiding this comment.
Out of curiosity, which version of Inkscape are you using? I think this may be a problem with newer versions and holding back a few versions older may solve the problem.
We're using 1.2.2 in the CI builds (see .github/workflows/main.yml). I've been remiss about adding this to the version information for Inkscape in the section above.
|
|
||
| ***1. Inkscape Errors:*** Gaffer utilizes Inkscape purely during the compilation phase to automatically convert all of the UI icon assets (.svg vector files) into .png raster graphics for the application layouts. But the problem is that even though Inkscape might be in the environment path scons will not find it. So we have to force it in the command line. The line to add at the end of the build command is `INKSCAPE="C:\Program Files\Inkscape\bin\inkscape.exe" --config=force` | ||
|
|
||
| ***2. Scons cannot see installed git:*** Same issue with Inkscape we'll need to create a symbolic link to the real git for scons to see. Open power shell in elviated privilages |
There was a problem hiding this comment.
This one is interesting, I wonder if this is something that's a problem in the Scons environment. Are you able to run git in the terminal you're building from, something like git -v to print version information? Is the git binary directory in your PATH? (You can do echo $env:PATH in your Powershell terminal to check it).
| New-Item -ItemType SymbolicLink -Path "C:\src\gaffer-build\bin\git.exe" -Target "C:\Program Files\Git\bin\git.exe" | ||
| ``` | ||
|
|
||
| ***3. GLEW.lib:*** The linker will blow up looking for GLEW.lib, the fix is to find the real file, and create a mirror copy named exactly what the linker wants. |
There was a problem hiding this comment.
I think this, and possibly some of the Inkscape stuff above, can be helped by using the sconsOptions file. In the CI build, we add OPTIONS=.github/workflows/main/sconsOptions to the command to set some Scons options.
The key one in that options file for GLEW is GLEW_LIB_SUFFIX = "32" That should point your build to the right version and not need to horse around with hard-linking.
| New-Item -ItemType HardLink -Path "C:\src\gaffer-build\lib\GLEW.lib" -Value "C:\src\gaffer-build\lib\glew32.lib" | ||
| ``` | ||
|
|
||
| ***4. Build Time:*** By default, SCons acts conservatively and compiles everything on a single CPU thread. You need to explicitly tell it to spin up parallel workers using the -j (jobs) flag. ***Note: switching to multi core build will error out when reaching the Inkscape stage. Because you split the build into multiple parallel threads using `-j 8`, Gaffer launched 8 separate instances of Inkscape simultaneously, all trying to access the exact same SVG file at once. Inkscape on Windows often handles parallel thread calling very poorly, resulting in memory overflows and crashes. Since you only need to build these icons once (and they don't change when you tweak your RenderMan C++ node code), you can bypass this multi-threaded bug easily.Because SCons tracks file generation states, it will not recompile any of your C++ code. All your RenderMan and Gaffer UI objects are already safely compiled as .obj and .dll binaries from your last run. So make sure to run the build again with `-j 1` when the build errors out on the icons |
There was a problem hiding this comment.
Hopefully this can be simplified by using the sconsOptions file I mention above, along with a better-working Inkscape. That options file will also take care of setting up RENDERMAN_ROOT, ARNOLD_ROOT, and DELIGHT_ROOT.
That options file does require having 3Delight and ONNX installed and configured, which can be done with Python scripts in the .github/workflows/main directory.
|
Thanks Eric, I'll cherry pick the nodal changes and submit another PR. I'll check your notes about the README and respond later. |
3 participants
Generally describe what this PR will do, and why it is needed
This PR is to reduce the display of RenderMan Stylized Looks Shading nodes, as these nodes are used heavily currently they expose every parameter as an input node. I've made changes to the visible ports by default based on usage.
Related issues
Dependencies
Breaking changes
Checklist