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

Store Node properties in the Node Tree #385

Merged
merged 20 commits into from
Aug 31, 2022
Merged

Conversation

pragma37
Copy link
Member

@pragma37 pragma37 commented Aug 24, 2022

This branch stores the node malt_properties inside the node tree malt_properties as discussed here:
#348 (comment)

This has 2 main advantages: scene loading performance and supporting drivers for node properties.

Don't save important files with this branch yet.

@pragma37 pragma37 changed the base branch from Development to node-ux August 24, 2022 21:20
@pragma37 pragma37 requested a review from Kolupsy August 24, 2022 21:20
@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 25, 2022

that was fast! looks good so far. I will be able to take a look some time later (hopefully today)


def copy(self, node):
#Find the node from its node tree so we have access to the node id_data
for tree in bpy.data.node_groups:
Copy link
Collaborator

Choose a reason for hiding this comment

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

does not find the node when you copy using ctrl-c / ctrl-v (for whatever reason). Therefore node.id_data stays None and fails the rest of the copy function. This is especially crucial when copying from one node tree to another. I have not found a solution to this particular problem though. It might be necessary to take the node tree out of the equation when copying nodes which adds a whole other level of complexity for sure

Copy link
Collaborator

Choose a reason for hiding this comment

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

possible solutions:

  1. if the only thing we need from the node tree is to get copies of the parameters maybe storing a copy of them at all times on the node itself (as a custom dict attribute or something) might be the way to do
  2. going back to storing the attributes themselves on the node but this time as custom properties DIRECTLY on the node to keep the possibility to add drivers
  3. basically the same as 2) but maybe instead of on the node they could be stored on the sockets instead

Copy link
Member Author

@pragma37 pragma37 Aug 25, 2022

Choose a reason for hiding this comment

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

I found an easier fix.
It won't work for copies across blend files, but I guess that's not that bad.

Your proposed solutions would solve that too, but maintaining a fully synced copy of all node parameters would be a maintenance hell, and storing attributes directly in the node or the socket would require a full rewrite of the properties system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeah I just saw. Could this be used as an identifier to fetch the parameters in the first place? because of the issue where parameters get reset when renaming a node?

Copy link
Member Author

@pragma37 pragma37 Aug 25, 2022

Choose a reason for hiding this comment

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

The problem would be that the internal parameter name and the variable name in the generated source must match, so it would make the generated source code much less readable.

We would also need a different id generation method, but that's the lesser problem.

I think for renames we could simply store the current name in the msgbus callback and reset the callback every time there's a rename.
Then we can simply add a rename_property function in MaltPropertyGroup.
It's not super pretty, but doesn't sound too problematic.

Edit:
Actually, your suggestion may be a better option.
msgbus seems quite unreliable.

@pragma37
Copy link
Member Author

I've been testing this.
I've updated the material examples to the new nodes using this branch.
Seems like everything is right.

I think it's ready for a merge?

@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 29, 2022

I think it's ready for a merge?

I was getting errors running this branch on a new machine (actually a machine that we talked about eariler already that I was having issues with developing). I wrote about it here: #387
Unlike the other two people getting similar errors I am not on a linux distro but on Windows 10 with an RTX graphics card just like the PC that I used before. The graphics drivers are definitely different. Ill see if there is something I can do. Does malt / openGL have very specific requirements in this area apart from not having mac support?

@pragma37
Copy link
Member Author

@Kolupsy Can you check the new commit?
If it still doesn't work, post the full log.

This is an error on the source generation side, it's not really related to the driver or OpenGL.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 29, 2022

nothing has changed on my side. this is whats currently happening (and therefore before as well). This is still just one the one machine and not the other one. it would surprise me if the issue WASNT machine specific since I cannot explain this behavior otherwise 🤔
scenario 1: node-ux branch works flawlessly when opening a new file and switching to Malt.
...
scenario 2: this branch causes this error when opening a new file and switching to Malt:
malt 2022-08-29(16-38).log

@pragma37
Copy link
Member Author

pragma37 commented Aug 29, 2022

But does it work otherwise?

I get that error as well if I switch in the default Blender startup file to Malt, but everything works fine anyway.

The error is caused because there's a compilation triggered too early, but a second compilation is triggered again after the actual setup, so everything ends up working correctly.

I have to fix it, but it shouldn't stop Malt from working.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 29, 2022

But does it work otherwise?

When switching to malt it prints said error but it behaves fine from there on out. When using a malt startup file its annoying because i would have to manually reregister the addon every time i start blender

@pragma37
Copy link
Member Author

pragma37 commented Aug 29, 2022

I don't see why it would prevent Malt from working.

In fact, your log doesn't show any render time errors.
If the shader that ends up being used had errors it would print OpenGL errors every sample when trying to bind it.

Anyway, I've improved the node tree update system to avoid unnecessary early updates and I also updated the defaults.blend file so it already has all the new setup ready when imported.

Does that fix your issues?

@pragma37
Copy link
Member Author

When using a malt startup file its annoying because i would have to manually reregister the addon every time i start blender

I don't get this part. 🤔

@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 29, 2022

I don't get this part. 🤔

I think since malt is trying to do stuff too early it already fails during addon registration and usually when exceptions occur during addon registration blender will just give up and not enable the addon at all. That's why I need to re-enable the addon in this case. Haven't tried the latest commits yet though. Can take a look in a bit

@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 29, 2022

Does that fix your issues?

Yes fixes the issue above, awesome!

The only thing that remains is that I cannot have malt as the engine thats enabled by default when starting blender (what I tried to describe above). Basically think it coomes down to BlenderMalt not being able to get the bridge MaltPipeline.get_bridge() because it seems that in the beginning the method just returns None. For now I can live with not having malt as the active render engine on startup, but I will leave this info just in case.
PS. there seems to be something else messed up with my blender. It doesnt seem to save preferences correctly. That might have a great influence on the outcome and might not have anything to do with malt. we will see.

@pragma37
Copy link
Member Author

I still don't understand exactly what's the issue.

When you save your startup file with Malt as the active render engine, then the addon gets disabled in preferences???
But it only happens with this branch, not with the node-ux branch. Is that right?
Does it print any errors?

Have you tried on a clean portable Blender install with its own config folder and the addon downloaded from the Github releases?

@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 30, 2022

When you save your startup file with Malt as the active render engine, then the addon gets disabled in preferences???

Yep that is the weird thing.

But it only happens with this branch, not with the node-ux branch. Is that right?

The issue is the same for both branches

Does it print any errors?

Nope it just silently does nothing. It doesnt even try to load Malt (not BlenderMalt), at least I think it is not because with malt enabled by default it loads up blender suspiciously quickly. Usually it would take a bit longer with malt enabled by default

Have you tried on a clean portable Blender install with its own config folder and the addon downloaded from the Github releases?

Still have to do the test. First trying to solve the problem that is JUST blender itself, then see if I can gather some more useful information though tests

@pragma37
Copy link
Member Author

pragma37 commented Aug 30, 2022

Ok, that's weird indeed.

The issue is the same for both branches

So Development and Release work fine?

Things I would try to narrow down the issue:

  • Try a clean Blender install with its own config and the pre-built addon from Releases, as mentioned earlier.
  • Print something at the first line of register() in the addon init file, to see if it tries to start at all.
  • Remove stuff from bl_info, maybe the "non-standard" version is causing the issue?
  • If it actually starts, use the trace module to print what's going on.
  • If it doesn't, then modify the addon_utils module to see what's going on.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 30, 2022

So Development and Release work fine?

Have not tested those branches yet.

Either way as expected this had nothing to do with malt itself. Instead it was my fault / a weird quirk of app templates. Turns out that unless the app template has a userpref.blend file, it wont load user preferences correctly. I remember solving this by creating a symlink to reference the 'original' user preferences in the app template. I recreated this on the new machine and now that works.
So I guess what had happened in the end was that it would just load the factory preferences where malt - naturally - is not enabled. So no register issue or something along those lines.

Edit: I also did a fresh install of blender anyway (3.2.2) but I am pretty sure that was not part of the issue

@Kolupsy
Copy link
Collaborator

Kolupsy commented Aug 30, 2022

image

This is something thats really cool about this branch. The Depth of Field node finally has some utility because with drivers you have the option to use focus objects. The driver is outputting the distance between a camera object and an empty so it just works when looking through the camera at the moment. Maybe in the future you could expose the view camera and empties to the pipeline or something? to calculate the distance that way

@pragma37
Copy link
Member Author

You could take a target position input and set the driver on that.
Then just compute the distance between the camera position and the target position.

But yep, at some point I would like to include probes. empties, object names, groups, etc. in the scene data so render nodes have access to them.

@pragma37 pragma37 merged commit 1f852b4 into node-ux Aug 31, 2022
@pragma37 pragma37 deleted the node-tree-properties branch August 31, 2022 16:59
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