Fix #116809: Geometry nodes UV map of UV Sphere #116856

Open
Balázs Füvesi wants to merge 15 commits from fuvesib/blender:gn-uvsphere-uvsegmentfix into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
First-time contributor

Fix #116809. The UV map of the UV Sphere Mesh Primitive matches other Geometry Nodes Mesh Primitives by shifting all the segments to the right and moving the last segment to the beginning.

Fix #116809. The UV map of the UV Sphere Mesh Primitive matches other Geometry Nodes Mesh Primitives by shifting all the segments to the right and moving the last segment to the beginning.
Balázs Füvesi added 1 commit 2024-01-07 00:46:47 +01:00
Iliya Katushenock added this to the Nodes & Physics project 2024-01-07 10:28:08 +01:00
Iliya Katushenock added the
Interest
Geometry Nodes
label 2024-01-07 10:28:12 +01:00
Iliya Katushenock reviewed 2024-01-07 11:00:41 +01:00
Iliya Katushenock left a comment
Member

Thanks for fix! Just one comment to make math more clear.

This fix works correct right now:
image

Now here is issue: old blend file (with original uv) is become to looks different.
This is compatibility issue. To fix that, you need to create new function in blender\blenloader\intern\versioning_400.cc.
This function need to be called at versioning_400.cc#L2561 line (with version check if statement). See this function as example versioning_300.cc#L553

In result, i think, is enough to rotate mesh to in old files to make it looks the same as in older blender. Here is my setup:
image
(I've highlighted in pencil below the new nodes that need to be created by the versioning code.)
If the to Segments socket of node previously a link was connected, then you need to create a new link so that it is connected to the Math node.
If Mesh output was unused, new nodes is not necessary. But in case is was used, links should be reconnected to be from Transform Geometry node, and new link from Mesh output to Transform Geometry input need to be created.

Thanks for fix! Just one comment to make math more clear. This fix works correct right now: ![image](/attachments/654c7730-c6ff-4533-804d-bcc450c14579) Now here is issue: old blend file (with original uv) is become to looks different. This is compatibility issue. To fix that, you need to create new function in `blender\blenloader\intern\versioning_400.cc`. This function need to be called at [versioning_400.cc#L2561](https://projects.blender.org/blender/blender/src/branch/main/source/blender/blenloader/intern/versioning_400.cc#L2561) line (with version check if statement). See this function as example [versioning_300.cc#L553](https://projects.blender.org/blender/blender/src/branch/main/source/blender/blenloader/intern/versioning_300.cc#L553) In result, i think, is enough to rotate mesh to in old files to make it looks the same as in older blender. Here is my setup: ![image](/attachments/ae636f18-cc07-4a8b-b4ba-3fc70565b80f) (I've highlighted in pencil below the new nodes that need to be created by the versioning code.) If the to `Segments` socket of node previously a link was connected, then you need to create a new link so that it is connected to the Math node. If `Mesh` output was unused, new nodes is not necessary. But in case is was used, links should be reconnected to be from `Transform Geometry` node, and new link from `Mesh` output to `Transform Geometry` input need to be created.
@ -252,3 +252,3 @@
for (const int i_segment : IndexRange(segments)) {
const int loop_start = i_segment * 3;
const float segment = float(i_segment);
float segment = float(i_segment + 1);

This could be done in single line:
const float segment = math::mod<int>(i_segment + 1, segments);.
It is better to make this in single line to keep segment variable as const.
If you will get error due to undefined function, you would search required header in blenlib folder.

This could be done in single line: `const float segment = math::mod<int>(i_segment + 1, segments);`. It is better to make this in single line to keep `segment` variable as const. If you will get error due to undefined function, you would search required header in `blenlib` folder.

I just made pretty similar versioning, you could use this as example: #116870/files

I just made pretty similar versioning, you could use this as example: [#116870/files](https://projects.blender.org/blender/blender/pulls/116870/files)
Balázs Füvesi added 1 commit 2024-01-08 00:03:20 +01:00
fe92ce299e Fix vertex position instead of UV shift
The UV shift solution introduces a compatibility issue:
If the sphere is rotated by one segment in versioning then the UV is consistent.
But the vertex positions are rotated.

The vertex position solution introduces a compatibility issue too:
If the sphere is rotated by one segment in versioning then the UV is consistent.
And the vertex positions are consistent too.
With the added benefit that the UV sphere vertex order matches the vertex order of cone and cylinder.
Balázs Füvesi added 1 commit 2024-01-08 00:09:56 +01:00
9f81894ea2 Add dummy version check
The current version just ensures that the function is called when my blend file is opened.
The correct version has to be set.
Balázs Füvesi added 1 commit 2024-01-08 00:14:01 +01:00
452b2f2090 Add nodes until segment input
Most of the nodes are done, but the segment input for UV sphere is not finished yet.
Lines 1862 - 1877 marked with TODO have to be completed.
Balázs Füvesi added 1 commit 2024-01-08 16:22:07 +01:00
Balázs Füvesi added 1 commit 2024-01-08 16:22:54 +01:00
Balázs Füvesi added 1 commit 2024-01-08 16:29:12 +01:00
Iliya Katushenock reviewed 2024-01-08 16:32:46 +01:00
@ -1805,0 +1881,4 @@
// nodeAddLink(&ntree, segments_node, integer_output, primitive_node, segments_input);
// nodeAddLink(&ntree, segments_node, integer_output, divide_node, divisor_input);
*version_cycles_node_socket_float_value(divisor_input) = *version_cycles_node_socket_int_value(segments_input);

version_cycles_node_socket_int_value->default_value_typed<bNodeSocketValueInt>()->value

`version_cycles_node_socket_int_value->default_value_typed<bNodeSocketValueInt>()->value`
fuvesib marked this conversation as resolved
Balázs Füvesi added 1 commit 2024-01-08 21:09:42 +01:00
Balázs Füvesi added 1 commit 2024-01-08 21:25:25 +01:00
Balázs Füvesi added 1 commit 2024-01-08 21:32:45 +01:00
Author
First-time contributor

Fix UV map of UV sphere to match cone's and cylinder's UV map by rotating vertex positions and matching them with cone's and cylinder's vertex positions (Small icosphere marks 1st vertex):

Old New
Screenshot 2024-01-08 at 21.13.09.png Screenshot 2024-01-08 at 21.13.26.png

Versioning:

Old Converted
Screenshot 2024-01-08 at 21.11.07.png Screenshot 2024-01-08 at 21.12.35.png
Fix UV map of UV sphere to match cone's and cylinder's UV map by rotating vertex positions and matching them with cone's and cylinder's vertex positions (_Small icosphere marks 1st vertex_): > | Old | New | > | -- | -- | > | ![Screenshot 2024-01-08 at 21.13.09.png](/attachments/365ff1de-1be8-4fc9-a34d-bd9fa0303c3b) | ![Screenshot 2024-01-08 at 21.13.26.png](/attachments/2030636a-3f69-49a7-adaa-810edf0b4dea) | Versioning: > | Old | Converted | > | -- | -- | > | ![Screenshot 2024-01-08 at 21.11.07.png](/attachments/e8ed4300-067b-41d4-9041-e1f2703218a4) | ![Screenshot 2024-01-08 at 21.12.35.png](/attachments/6b887efc-3fee-4884-a2cd-7039a2ffbb2f) |
Iliya Katushenock approved these changes 2024-01-09 12:35:36 +01:00
Iliya Katushenock left a comment
Member

Now everything is works correct! Old file looks the same, and easy i can mute Transform Geometry and result will be fixed. I add some comments about code clean.

Now everything is works correct! Old file looks the same, and easy i can mute `Transform Geometry` and result will be fixed. I add some comments about code clean.
@ -1805,0 +1809,4 @@
LISTBASE_FOREACH (bNodeLink *, link, &ntree.links) {
bNode *to_node = link->tonode;
if ((to_node->type == GEO_NODE_MESH_PRIMITIVE_UV_SPHERE) && (link->tosock == nodeFindSocket(to_node, SOCK_IN, "Segments"))) {

You can simply check name of socket (STREQ(link->tosock->name, "Segments")). Same below.

You can simply check name of socket (`STREQ(link->tosock->name, "Segments")`). Same below.
fuvesib marked this conversation as resolved
@ -1805,0 +1813,4 @@
primitive_nodes_inputs.add(to_node, link);
}
bNode *from_node = link->fromnode;
if ((from_node->type == GEO_NODE_MESH_PRIMITIVE_UV_SPHERE) && (link->fromsock == nodeFindSocket(from_node, SOCK_OUT, "Mesh"))) {

Can you run in your root folder of blender fork:

git diff main --name-only
make format FILES-CHANGED-IN-BRANCH

To format this and other lines. See https://wiki.blender.org/wiki/Tools/ClangFormat

Can you run in your root folder of blender fork: ``` git diff main --name-only make format FILES-CHANGED-IN-BRANCH ``` To format this and other lines. See https://wiki.blender.org/wiki/Tools/ClangFormat
fuvesib marked this conversation as resolved
@ -1805,0 +1864,4 @@
bNodeSocket *divisor_input = static_cast<bNodeSocket *>(BLI_findlink(&divide_node->inputs, 1));
bNodeSocket *value_output = nodeFindSocket(divide_node, SOCK_OUT, "Value");
*version_cycles_node_socket_float_value(dividend_input) = M_PI * 2;
`2` -> `2.0f` See https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Value_Literals
fuvesib marked this conversation as resolved
@ -1805,0 +1870,4 @@
if (primitive_nodes_inputs.contains(primitive_node)) {
bNodeLink *link = primitive_nodes_inputs.lookup(primitive_node);
nodeAddLink(&ntree, link->fromnode, link->fromsock, divide_node, divisor_input);

Add continue; in end of this branch, and delete else after. This is early return pattern (to reduce nesting).

Add `continue;` in end of this branch, and delete `else` after. This is early return pattern (to reduce nesting).
fuvesib marked this conversation as resolved
@ -344,6 +344,12 @@ void add_realize_instances_before_socket(bNodeTree *ntree,
}
}
int *version_cycles_node_socket_int_value(bNodeSocket *socket)

This function doesn't make sense. Here is bNodeSocket::default_value_typed<bNodeSocketValueInt>() method witch is should be used to access to the socket value. This is commonly used in whole blender.

This function doesn't make sense. Here is `bNodeSocket::default_value_typed<bNodeSocketValueInt>()` method witch is should be used to access to the socket value. This is commonly used in whole blender.

If you think you done my comment point, you would press Resolve conversation.

If you think you done my comment point, you would press `Resolve conversation`.
fuvesib marked this conversation as resolved
Iliya Katushenock requested review from Hans Goudey 2024-01-09 12:36:18 +01:00
Balázs Füvesi added 3 commits 2024-01-10 00:06:41 +01:00
Balázs Füvesi added 1 commit 2024-01-11 19:53:35 +01:00
Balázs Füvesi added 1 commit 2024-01-11 20:01:38 +01:00
Member

Thanks for working on this. Maybe I don't understand it properly, but I have to say I'm not convinced by the versioning strategy. It seems like rotating the whole sphere is a more invasive change than fixing the UV map. I'd expect that many more files depend on the vertex positions than the UV map values of the sphere's top. I'm not so sure about better options yet.

Thanks for working on this. Maybe I don't understand it properly, but I have to say I'm not convinced by the versioning strategy. It seems like rotating the whole sphere is a more invasive change than fixing the UV map. I'd expect that many more files depend on the vertex positions than the UV map values of the sphere's top. I'm not so sure about better options yet.

Only thing why files might be changed is if files depends on indices of mesh. Right now positions looks the same.

Only thing why files might be changed is if files depends on indices of mesh. Right now positions looks the same.
Author
First-time contributor

Maybe I'm misunderstanding something, but with the current fix and versioning all three (UVMap, index and position) are the same, see the picture below.
With the added benefit that the vertex order follows the cylinder and cone primitives' vertex order.
Just by fixing the UV map and not rotating the sphere, I didn't find a way to achieve all of these.

Old New with versioning New Cylinder (old/new Fill type Triangles)
old.png newVersioning.png new.png cylinder.png

Small icosphere marks the 1st, small cube the 2nd vertex, 0th is on top, last on bottom.
Note with versioning the old and new UVMap, index and position are identical.
The new has identical UVMap and vertex order with the other primitives e.g. cylinder.

Maybe I'm misunderstanding something, but with the current fix and versioning all three (UVMap, index and position) are the same, see the picture below. With the added benefit that the vertex order follows the cylinder and cone primitives' vertex order. Just by fixing the UV map and not rotating the sphere, I didn't find a way to achieve all of these. | Old | New with versioning | New | Cylinder (old/new Fill type Triangles) | | -- | -- | -- | -- | | ![old.png](/attachments/88db74f7-673f-4134-a679-234f8e92bfa4) | ![newVersioning.png](/attachments/fce236d5-3cf7-46a6-8b91-eb860e23fd53) | ![new.png](/attachments/50701cce-5ade-4229-864b-b14a836aef5f) | ![cylinder.png](/attachments/958fc176-036d-4eee-a515-9adab648e3da) | _Small icosphere marks the 1st, small cube the 2nd vertex, 0th is on top, last on bottom._ Note with versioning the old and new UVMap, index and position are identical. The new has identical UVMap and vertex order with the other primitives e.g. cylinder.
This pull request has changes conflicting with the target branch.
  • source/blender/blenkernel/BKE_blender_version.h
  • source/blender/blenloader/intern/versioning_400.cc

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u gn-uvsphere-uvsegmentfix:fuvesib-gn-uvsphere-uvsegmentfix
git checkout fuvesib-gn-uvsphere-uvsegmentfix
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#116856
No description provided.