This replaces the old Attribute Map Range node.
This also adds the Shader version of the node.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- master
- Build Status
Buildable 19379 Build 19379: arc lint + arc unit
Event Timeline
Comment Actions
Personally I would much prefer having a type drop-down in the existing map range node than a new node for this data type.
This approach just doesn't scale well in my opinion. When we want to deal with 2D vectors or other data types, or when we want to use type inference to set the type of a node correctly it just doesn't work so well.
I know @Charlie Jolly (charlie) mentioned there were some technical difficulties with that. Especially that EEVEE isn't able to ignore unused sockets currently. This seems like a more general problem to fix though, IMO it's not worth compromising on the design to make that work.
Comment Actions
I tried to have the node spilt according to data type but it seems Eevee builds the function signatures using all sockets including ones that are flagged as unavailable. It wasn't trivial for me to fix without breaking other nodes. I guess it's a good problem to solve. Unless I'm missing an easy fix.
Comment Actions
For Eevee the simple workaround is to add all sockets as function parameters, and just not use some of them in the float and vector variations. GLSL compilers will optimize them out.
Comment Actions
The current way nodes are designed is not good for switching data types without having a lot of code, especially when there are a lot of sockets. Having less code in the node is easier to maintain. Maybe there is a better way for a user to select Map Range and switch between data types. Python side switching might be a better solution like Node wrangler rather than in the node itself.
Comment Actions
Update Map Range node to support vectors. Ready for design comments rather than code review.
Compared to previous patch:
- Data type is selectable in node
- Nodeclass is runtime and can't be set for each mode independently (either blue or purple)
@Brecht Van Lommel (brecht) @Hans Goudey (HooglyBoogly) what do you think compared to previous patch with separate node?
Comment Actions
The shader code is certainly less beautiful, but I think this is definitely the way to go. When we have 2D vector sockets, they can work here too.
I don't think it would ever make sense to have "Map Range", "2D Vector Map Range", "Vector Map Range", (Integer map range?) etc. when they are the same concept.
Comment Actions
Cycles / Eevee part of this looks fine to me, except for one thing that is easily fixed.
| intern/cycles/blender/shader.cpp | ||
|---|---|---|
| 381 | Use BL::ShaderNodeMapRange::data_type_FLOAT_VECTOR instead of 48. | |
Comment Actions
- Comments inline should be simple to resolve.
- This should be updated with the changes from rB2309fa20af416d47
| source/blender/nodes/shader/nodes/node_shader_map_range.cc | ||
|---|---|---|
| 40–44 | The socket name should be the same as the float inputs, instead the socket identifier should be tweaked. node_fn_compare.cc did that most recently (though looking at it now, I think I'd prefer "FLOAT3" over "VEC3" | |
| 41 | Might be simplest to write this like .default_value(float3(1)); etc. | |
| 94 | "NodeMapRange" -> __func__ | |
| 208–216 | I'd prefer if these were split up into one or two functions that operated on float3. | |
Comment Actions
Address comments from review, thanks.
Sticking with blue, it is not currently possible to change the node type at runtime without affecting all Map Range nodes.
Comment Actions
This looks good to me now.
Do you have any test files from your development? It would be good to make them into regression tests.