The issue was the custom loop normal data would be mangled when we reversed the face loops.
The flip face code will now correctly flip the custom face normals so they are not left in an undefined state.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Welll..... Main issue I can see here is that you are inverting all normals, when the face flipping code may be applied to only some (selected) faces...
TBH am not so sure that we want to support that kind of things, custom normals are by design not resilient to topology changes. Supporting them in some cases makes sense, but here, with the possibility of getting opposed normals and such, imho getting a satisfying behavior requires a fair amount of work, and is not really worth it?
Comment Actions
This is not the case, only the selected faces will get their custom normals inverted. I have tested this on a few different setups with split and merged normals. This seems to work fine in the cases I tested.
TBH am not so sure that we want to support that kind of things, custom normals are by design not resilient to topology changes. Supporting them in some cases makes sense, but here, with the possibility of getting opposed normals and such, imho getting a satisfying behavior requires a fair amount of work, and is not really worth it?
The only thing needed to support flipping is the code I've added here. So I do not see an issue with supporting flipping custom normals as well.
| source/blender/bmesh/operators/bmo_utils.c | ||
|---|---|---|
| 141–142 ↗ | (On Diff #24067) | Can you explain me how this is not inverting all clnors? |
Comment Actions
Okay, so had forgotten that BM_loop_normal_editdata_array_init is smart and uses selection to detect which loops normals to edit.
But the root issue remains, there is no guarantee that current selection matches the faces parameter this BMO is called with. In fact, it is never called from BMesh code itself, only from higher-level, caller code from editor area. And I think it should stay that way.
So I'd say, general code is OK, but handling of clnors should be done from edbm_flip_normals_exec instead. Also because reverse_faces BMO is called from other places and I'm not sure at all dealing with clnors there would not conflict with other things then.
Comment Actions
I've updated the patch so it now works reliably.
My first iteration of this patch must have used some undefined behavior as it just suddenly stopped working.
Things that are still TODO:
Add this to the "recalculate normals" function too as it has a "flip" option in it as well.- Perhaps I can expand this so it works when you mark/unmark edges as sharp via the operator too? (Now it mangles the custom loop normals when marking as sharp)
If I have this implemented too, I think I might actually be able to unmark edges as sharp when flipping back and forth.
Now sharp edges remain even after flipping the face back.
Or perhaps we should add a mode/tick box where it only flips the custom normals?
Comment Actions
Generally looks like going in the right direction, but don't think that patch is ready yet.
TBH, I would not try to support the 'custom normals only' option, this is not going to be easy, and I have doubt about actual added value of that option?
If you simply invert clnors of inverted faces, then code here can be used pretty easily (don't think you even have to deal with manually tagging edges as sharp, since winding-opposite faces always imply sharp edges between them)...
| source/blender/editors/mesh/editmesh_tools.c | ||
|---|---|---|
| 2015 | shouldn't this be rather lnors_ed_arr_new_full->lidx_to_lnor_editdata[lnor_ed->loop_index]; ? | |
| 2024–2040 | Don't think this is correct, and pretty sure this is not needed:
| |
| 2046 | I'd put an assert here that lnor_ed is actually valid, to ensure there is no issue in logic of code above. | |
| 2052 | That kind of implicit behavior should really be avoided... caller should free that one (either explicitly, or even better, calling a flip_custom_normals_release_data() or something like that, that would actually do the job. | |
| 2058 | naming (only_clnors?) | |
| 2069–2087 | You should never flip face normals if only_custom is true (so this should be to different tests, and always continue in any case when only_custom is true)... | |
| 2069–2089 | This cannot be valid? In that case, you do need to generate sharp edges manually (unlike in case where faces are also inverted, since then clnor spaces are split automatically)... | |
| 2091–2098 | has_flipped_faces | |
| 2125 | please use meaningful name, like only_custom_normals, or only_clnors. Label could be shortened too, Custom Normals Only e.g., and add proper description in tooltip. | |
| 2416–2421 | Not sure if this is working at all? Previous call to recalc_face_normals BMO would have already messed clnors completely if it did change something... In fact, I would wrap both BMO calls with those filp_custom_normals process... | |
Comment Actions
I've updated the patch with your feedback except for the "sharp edges" part.
I think we might need to discuss it a bit more.
I added the extra option only_clnors because then it will not modify the state of the normals (if they merged or split).
If this option is not on, we will split the normals as needed so only the clnors belonging to the face will be flipped.
This has an issue though, when we split normals this is fine, but then when we merge them, it seems that the loop indexes are not valid anymore and will lead to NULL pointers if we try to derive which older clnor corresponds to the new one. (But I might just be doing something wrong here).
So in this case I simply mark edges so that the normals stay split and it seems to work.
But I do not really think this is a good solution. It is just that I couldn't really think of how to map the normals if the lookup in lidx_to_lnor_editdata fails.
| source/blender/editors/mesh/editmesh_tools.c | ||
|---|---|---|
| 2416–2421 | Ah sorry! You are totally right. | |
Comment Actions
Disregard my previous post. I looked into it more and realized that I was wrong! :)
Now I do not mark edges as sharp and the loop normals seems to be properly inverted.
| source/blender/editors/mesh/editmesh_tools.c | ||
|---|---|---|
| 2069–2089 | The intention here is to not modify and properties of the custom loop normals. Perhaps we should move this to an other operator? | |
Comment Actions
Besides details noted below, things look good to me now.
| source/blender/editors/mesh/editmesh_tools.c | ||
|---|---|---|
| 2015 | lnor_ed_new_full is never going to be NULL here, should check for lnor_ed... | |
| 2039 | Should check for both, also lnor_ed_new here... | |
| 2406 | This should be read only once, and stored in a local static bool. | |
| 2407 | flip them ;) | |
Comment Actions
@Bastien Montagne (mont29) I guess you are ok with me pushing this and rBa5d394fad23280687880aee0082797cf8dd1cdd5 (follow up fix) to the 2.83 branch?
Comment Actions
@Sebastian Parborg (zeddb) Am certainly NOT OK with pushing this in 2.83, this is not even a bug fix (rather a new feature), definitively not a regression, and everything but safe fix. It is for master only!
Comment Actions
@Bastien Montagne (mont29) okay, I see.
Should I test it thorougly? Will it help to push it into upcoming 2.83 release?
Comment Actions
@Vyacheslav (hitrpr) the more testing the better, but there is zero chance for this to make it into 2.83, which should be released next week hopefully. There is just no time for that anymore.