Production File broke between LTS releases #113018

New Issue

Simon Thommes · 2023-09-28T16:07:31+02:00

Simon Thommes commented

2023-09-28 16:07:31 +02:00

Blender Version
Broken: version: 3.6.3, 4.0 beta, 4.1 alpha
Worked: 3.6.2

Short description of error

Exact steps for others to reproduce the error
File:
https://studio.blender.org/films/wing-it/3c308f54ee719e/?asset=6953

**Blender Version** Broken: version: 3.6.3, 4.0 beta, 4.1 alpha Worked: 3.6.2 **Short description of error** ![image](/attachments/f6905482-e482-42a7-9a37-0ba8e30935cf) **Exact steps for others to reproduce the error** File: https://studio.blender.org/films/wing-it/3c308f54ee719e/?asset=6953

image.png

478 KiB

Simon Thommes added the

labels 2023-09-28 16:07:32 +02:00

Dalai Felinto commented

2023-09-28 17:33:42 +02:00

It is broken on 3.6.3 already even ~~(I will update the description)~~.

It is broken on 3.6.3 already even ~~(I will update the description)~~.

Dalai Felinto added

and removed

labels 2023-09-28 17:34:15 +02:00

Dalai Felinto commented

2023-09-28 17:40:30 +02:00

And the issue is present both in main (4.1) and 4.0 branches.

Simon Thommes commented

2023-09-28 17:41:30 +02:00

I spoke with @JacquesLucke
This seems to be due to some changes in the edge split node that were backported to fix some bugs and cause a difference index order.
Since this setup relies on a consistent index order it breaks. I was told that we should just not rely on index order, which is generally fair, while I still think there's a point to be made to put in an effort to still keep this reliable between LTS versions.
Regardless, we can adjust the setup in the files to not rely on index order and use UV sampling instead. But this brings up a general discussion that if we should never rely on indices for data transfer (except for cases where that can always be ensured, which is a bit fuzzy), there needs to be a better general solution in geometry nodes to transfer data between geometries, that is more robust.
UVs work great. But they require some manual management and the sampling is less efficient. Some similar ID attribute based transfer could work for transferring between discrete elements, rather than interpolating.

I spoke with @JacquesLucke This seems to be due to some changes in the edge split node that were backported to fix some bugs and cause a difference index order. Since this setup relies on a consistent index order it breaks. I was told that we should just not rely on index order, which is generally fair, while I still think there's a point to be made to put in an effort to still keep this reliable between LTS versions. Regardless, we can adjust the setup in the files to not rely on index order and use UV sampling instead. But this brings up a general discussion that if we should never rely on indices for data transfer (except for cases where that can always be ensured, which is a bit fuzzy), there needs to be a better general solution in geometry nodes to transfer data between geometries, that is more robust. UVs work great. But they require some manual management and the sampling is less efficient. Some similar ID attribute based transfer could work for transferring between discrete elements, rather than interpolating.

Jacques Lucke referenced this issue

2023-09-28 22:55:45 +02:00

Geometry: add utility to check for bad geometry element index dependence #113030

Hans Goudey commented

2023-09-29 02:40:55 +02:00

Indeed, this isn't a bug, since these indices from geometry processing can change between Blender versions. Obviously it's far better if we don't change them between LTS point releases. I spent a while trying to avoid that here, but ended up only being confident with a more complete rewrite that I understood properly (be2df46afe).

Indeed, this isn't a bug, since these indices from geometry processing can change between Blender versions. Obviously it's far better if we don't change them between LTS point releases. I spent a while trying to avoid that here, but ended up only being confident with a more complete rewrite that I understood properly (be2df46afe58a662b2d0308906a9927e72f4629a).

Hans Goudey closed this issue

2023-09-29 02:40:58 +02:00

Blender Bot added

and removed

labels 2023-09-29 02:41:00 +02:00

Dalai Felinto commented

2023-09-29 16:46:09 +02:00

Obviously it's far better if we don't change them between LTS point releases.

I know it is not an easy call to make either way, but I would go a step further and be more strict on what passes as a LTS-acceptable fix.

While I understand a bug fix leading to changes in files, I don't think it is ok in-between LTS`. And all-in-all I would treat the original 3.6.0 Blender as the reference point (to avoid the argument that if we revert the fix, we would once again be breaking things).

From looking at be2df46afe the situation is not as simple as reverting something though. It feels like there were a bunch of commits leading to the final solution?

My question is, what would be the consequence of reverting the fix? How "bad" the original issue was? How doable it is to pin-point one commit to make the production file to work as before?

Just to illustrate my point, I was literally yesterday recommending an LTS upgrade for someone using 3.6.1 and experiencing crashes. Upgrading from 3.6.1 to 3.6.4 should be a no-brainer.

> Obviously it's far better if we don't change them between LTS point releases. I know it is not an easy call to make either way, but I would go a step further and be more strict on what passes as a LTS-acceptable fix. While I understand a bug fix leading to changes in files, I don't think it is ok in-between LTS`. And all-in-all I would treat the original 3.6.0 Blender as the reference point (to avoid the argument that if we revert the fix, we would once again be breaking things). From looking at be2df46afe58 the situation is not as simple as reverting something though. It feels like there were a bunch of commits leading to the final solution? My question is, what would be the consequence of reverting the fix? How "bad" the original issue was? How doable it is to pin-point one commit to make the production file to work as before? --- Just to illustrate my point, I was literally yesterday recommending an LTS upgrade for someone using 3.6.1 and experiencing crashes. Upgrading from 3.6.1 to 3.6.4 should be a no-brainer.

Hans Goudey commented

2023-09-29 16:50:51 +02:00

If revert the fix, the split edges node will skip loose edges again (it will also be much slower, but that's probably not so relevant). I think that's a worse situation than changes in indices, which people don't often depend on, and the manual even recommends against it.

Jacques Lucke referenced this issue from a commit

2023-09-29 21:44:45 +02:00

Geometry: add utility to check for bad geometry element index dependence

Simon Thommes commented

2023-10-02 10:14:43 +02:00

the split edges node will skip loose edges again (it will also be much slower, but that's probably not so relevant)

I honestly don't see why that would be an issue if that's already how the behavior for 3.6.2 was. That doesn't sound like a bug to me. The edit mode operator also skips loose edges afaict. Can you point me to the issue where this was discussed, so I can understand better?

> the split edges node will skip loose edges again (it will also be much slower, but that's probably not so relevant) I honestly don't see why that would be an issue if that's already how the behavior for 3.6.2 was. That doesn't sound like a bug to me. The edit mode operator also skips loose edges afaict. Can you point me to the issue where this was discussed, so I can understand better?

Hans Goudey commented

2023-10-02 13:23:46 +02:00

#109236

It's definitely a bug, and a bad one IMO. People relied on that behavior when dealing with curves. The edit mode operator doesn't skip loose edges, I just tested it.

#109236 It's definitely a bug, and a bad one IMO. People relied on that behavior when dealing with curves. The edit mode operator doesn't skip loose edges, I just tested it.

Simon Thommes commented

2023-10-02 14:25:34 +02:00

Ah, thanks for clarifying. Looks like I had a misunderstanding what should happen with splitting loose edges. And yea, if it's a regression then I can see how it's a bug

Joseph Eagar referenced this issue from a commit

2023-10-07 21:46:16 +02:00

Geometry: add utility to check for bad geometry element index dependence

Joseph Eagar referenced this issue from a commit

2023-10-07 21:46:17 +02:00

Geometry: add utility to check for bad geometry element index dependence

Sign in to join this conversation.