GPUOpen-LibrariesAndSDKs / RadeonProRenderBlenderAddon

This hardware-agnostic rendering plug-in for Blender uses accurate ray-tracing technology to produce images and animations of your scenes, and provides real-time interactive rendering and continuous adjustment of effects.
https://gpuopen.com/radeon-prorender-suite/
Apache License 2.0
480 stars 56 forks source link

Fix: ShaderNodeMapping Rotation Input Value (Fixes #606) #607

Closed jombo23 closed 1 year ago

jombo23 commented 1 year ago

PURPOSE

Fixes the Vector Mapping Rotation input node not working.

EFFECT OF CHANGE

The Vector Mapping Rotation input node will now work as expected

TECHNICAL STEPS

Changed "get_input_default" to "get_input_value" which polls the value on the input of the node instead of the default value in the node

NOTES FOR REVIEWERS

Tested Locally on a single project. Everything works as expected

jombo23 commented 1 year ago

Depending on the source, there are occasionally 3 and 4 element vectors intermixed for the rotation Input. Please check and change my method of handling this (slicing) if needed, as python is not my forte.

jombo23 commented 1 year ago

It has come to my attention that the rotation does not work if the input type relies on data from the object, such as random, etc, As it is referred to as a "MaterialNode", however it works with a handful of other sources and modifiers.

At this point I believe it is over my head, and needs to be corrected by someone else.

The core issue has at least been pointed out.

bsavery commented 1 year ago

Sorry, just to be clear, can you attach a scene where it is not working?

jombo23 commented 1 year ago

In which instance are you referring to it not working?

606 contains a scene where it doesnt work in the released RPR plugin. Are you asking for a scene in which my changes no longer work properly?

bsavery commented 1 year ago

Yes, a scene for what you describe above that this change doesn't work. We can take it from there.

jombo23 commented 1 year ago

Absolutely, here you go!

The error is achieved through attaching the "Object Info->Random" output in any way/shape/form to the "Vector->Mapping->Rotation" input

shadertestsproender.zip

Troublesome node..

image

bsavery commented 1 year ago

@VascoPi can you take a look?

odil24 commented 1 year ago

Absolutely, here you go!

The error is achieved through attaching the "Object Info->Random" output in any way/shape/form to the "Vector->Mapping->Rotation" input

shadertestsproender.zip

Troublesome node..

image

Can you paste console error for this node? It looks like this node has been changed (e.g.: added new options into the node) and you have tried to open previous blend file which has been this node when worked.

jombo23 commented 1 year ago

Replying from my phone.

Sure, I can paste the error later. please realize I colored the node red for the sake of being able to see it. it's not red because it's invalid. (I'm assuming you glanced at it and it just looks like it's an invalid node)

jombo23 commented 1 year ago

Adding the "Object Info->Randomness" output into the input of the "Vector->Mapping->Rotation" input in any way causes it to say: " sin_x, sin_y, sin_z = map(math.sin, rotation.data[0:3]) TypeError: 'MaterialNode' object is not subscriptable"

And if you remove my slicing (to chop the four element vectors down)

" sin_x, sin_y, sin_z = map(math.sin, rotation.data) TypeError: 'MaterialNode' object is not iterable"

Both referencing line ~2050 in blender_nodes.py (file referenced in PR)

Before the PR, the code calls get_input_default, which seems to ignore any nodes connected, and only accepts constants to be entered in on the node itself.

In any case, bringing the "Random" value in seems to change the incoming values type to MaterialNode instead of the value itself.

VascoPi commented 1 year ago

At the moment Rotation input ignors all links, only default input affect mapping.

image

Regarding errors that occurs in suggested solution it's because Random output (as well as many others) is MaterialNode type witch requires a bit different support than built-in python types. Basicaly the solution is correct but requires more work to handle MaterialNode.

I've made some changes to support MaterialNode. Let me know whether it is a desirable result:

image

jombo23 commented 1 year ago

Going by the screenshot it appears to be working like it is intended (and does in cycles/Eevee)!

jombo23 commented 1 year ago

I'm very curious as to what changes were required for pulling the correct information from the different type, but i can't seem to locate if/where you posted them. I checked your prorender branches, etc. I write embedded c, so I'm unfamiliar with the SOP of python, but I'd still like to have a look for the sake of me possibly being able to fix other bugs in the future.

VascoPi commented 1 year ago

I'm currently working on providing the solution and should be able to deliver it shortly.

VascoPi commented 1 year ago

https://github.com/VascoPi/RadeonProRenderBlenderAddon/tree/issue_606

Here's the branch. It requires review.

jombo23 commented 1 year ago

Aha! I was close in my own testing, as get_channel seemed like a candidate, but I didn't know that you could call the trig operators directly on the input itself.

Thanks a bunch! I will evaluate this in my own use cases and will post if I stumble across any other edge cases.

VascoPi commented 1 year ago

May I ask you to update the pull request with the provided changes, so others cam review it?

jombo23 commented 1 year ago

Absolutely!

I am on my phone currently but I will be at my workstation within the hour :)

jombo23 commented 1 year ago

PR Updated.

In my own testing scenarios everything works as expected!

VascoPi commented 1 year ago

It's ready for tests. @jombo23 could you please update with latest master?

jombo23 commented 1 year ago

@VascoPi Done