Fix #109491: Erratic behavior of materials for text objects #109746

Philipp Oeser · 2023-07-05T15:34:30+02:00

Philipp Oeser commented

2023-07-05 15:34:30 +02:00

Symptoms for this were most likely observed when removing material slots
(or moving them up/down).

For text objects, the CharInfo mat_nr material index starts at 1
(not at zero like for meshes or nurbs).
Code was mostly considering this (but not in all places, so material
index handling (removing/moving) could still go wrong.
This patch (hopefully) fixes the remaining places where the 1-based index
should be used.

In addition to that, new text objects will have their CharInfo
mat_nr index 1-based, places where the index is cleared also set it to
1 (instead of zero) now. This also includes version code to correct a
zero index for old files (and sets it to 1 as well).

Symptoms for this were most likely observed when removing material slots (or moving them up/down). For text objects, the `CharInfo` `mat_nr` material index starts at 1 (not at zero like for meshes or nurbs). Code was mostly considering this (but not in all places, so material index handling (removing/moving) could still go wrong. This patch (hopefully) fixes the remaining places where the 1-based index should be used. In addition to that, new text objects will have their `CharInfo` `mat_nr` index 1-based, places where the index is cleared also set it to 1 (instead of zero) now. This also includes version code to correct a zero index for old files (and sets it to 1 as well).

Philipp Oeser added 1 commit 2023-07-05 15:34:40 +02:00

Fix #109491 : Erratic behavior of materials for text objects af5988184f

Symptoms for this were most likely observed when removing material slots
(or moving them up/down).

For text objects, the `CharInfo` `mat_nr` material index starts at 1
(not at zero like for meshes or nurbs).
Code was mostly considering this (but not in all places, so material
index handling (removing/moving) could still go wrong.
This patch (hopefully) fixes the remaining places where the 1-based index
should be used.

In addition to that, new text objects will have their `CharInfo`
`mat_nr` index 1-based, places where the index is cleared also set it to
1 (instead of zero) now. This also includes version code to correct a
zero index for old files (and sets it to 1 as well).

Philipp Oeser added this to the Modeling project 2023-07-05 15:35:04 +02:00

Philipp Oeser requested review from Harley Acheson 2023-07-05 15:35:17 +02:00

Philipp Oeser requested review from Hans Goudey 2023-07-05 15:35:25 +02:00

Philipp Oeser requested review from Dalai Felinto 2023-07-05 15:35:33 +02:00

Philipp Oeser commented

2023-07-06 10:13:25 +02:00

Just noting that an alternative approach would be to change the whole thing to be zero-based instead (including version-patching old files). Much more involved, but could be considered (but maybe better be done separately).

Hans Goudey commented

2023-07-10 20:42:17 +02:00

These one based indices will always be confusing. If it's causing problems, personally I'd prefer to move to zero-based like you mentioned. Seems like 4.0 is also the ideal time to do that.

Harley Acheson commented

2023-07-19 21:36:11 +02:00

I probably just don't know these material details enough so my guesses are nonsense. But my understanding here is that this per-character CharInfo->mat_nr uses zero to indicate not set, and so a value of 1 will represent the first material, probably indexed as 0 elsewhere. But wouldn't that mean that initialization of this value should be to zero?

Philipp Oeser commented

2023-07-20 09:21:50 +02:00

I probably just don't know these material details enough so my guesses are nonsense. But my understanding here is that this per-character CharInfo->mat_nr uses zero to indicate not set, and so a value of 1 will represent the first material, probably indexed as 0 elsewhere. But wouldn't that mean that initialization of this value should be to zero?

I dont think there is really a concept of "not set" elsewhere in blender.
You have these at zero by default, so there is no distinction of the first material being assigned explicitly or never touched, first material will be used regardless.

So text objects should probably not be different here.

> I probably just don't know these material details enough so my guesses are nonsense. But my understanding here is that this per-character CharInfo->mat_nr uses zero to indicate not set, and so a value of 1 will represent the first material, probably indexed as 0 elsewhere. But wouldn't that mean that initialization of this value should be to zero? I dont think there is really a concept of "not set" elsewhere in blender. You have these at zero by default, so there is no distinction of the first material being assigned explicitly or never touched, first material will be used regardless. So text objects should probably not be different here.

Harley Acheson commented

2023-07-20 18:17:30 +02:00

I don't think there is really a concept of "not set" elsewhere in blender...So text objects should probably not be different here.

For sure it is weird. But I still worry that the behavior was chosen for some purpose and if we are not sure what that is then we might run into whatever it was that they were trying to avoid. Personally... I would limit this change to the referenced fix but then plan on a larger refactor later than changes the index to zero-based.

For report #109491 the only code of your changes that is hit is in BKE_curve_material_index_remove. We have three materials and remove the last one so the index coming in is 2 while our values in info->mat_nr are 1-3. We are off by one and your change there fixes it nicely.

But I would consider leaving the rest, including the versioning, to a refactor. That way this fix could go into a future 3.6 release while 4.0 could have these zero-based.

> I don't think there is really a concept of "not set" elsewhere in blender...So text objects should probably not be different here. For sure it is weird. But I still worry that the behavior was chosen for _some_ purpose and if we are not sure what that is then we might run into whatever it was that they were trying to avoid. Personally... I would limit this change to the referenced fix but then plan on a larger refactor later than changes the index to zero-based. For report #109491 the only code of your changes that is hit is in `BKE_curve_material_index_remove`. We have three materials and remove the last one so the index coming in is 2 while our values in `info->mat_nr` are 1-3. We are off by one and your change there fixes it nicely. But I would consider leaving the rest, including the versioning, to a refactor. That way this fix could go into a future 3.6 release while 4.0 could have these zero-based.

Philipp Oeser commented

2023-07-21 10:54:32 +02:00

For report #109491 the only code of your changes that is hit is in BKE_curve_material_index_remove. We have three materials and remove the last one so the index coming in is 2 while our values in info->mat_nr are 1-3. We are off by one and your change there fixes it nicely.

Thats where I started as well, so I get your point.
However, the report also mentions material slots moving up and down, so this would need more changes than the one in BKE_curve_material_index_remove and these involve material remapping, array permutation etc (see BKE_object_material_remap), which is not easily fixable with "arrays with holes". What I mean by that is that we dont actually have "values in info->mat_nr 1-3 " as you mentioned, but instead it is 0/2/3. Only if you specifically assign the first material to some letters, the mat_nr will change from 0 to 1 [which makes the whole situation more manageble, but cannot be required by the user]

But I would consider leaving the rest, including the versioning, to a refactor. That way this fix could go into a future 3.6 release while 4.0 could have these zero-based.

As much as I'd like to just have some short, backportable fix, leaving the rest of this patch out does not really fix it. Old files would still be broken. And since we already know the root of the issue, imho it only makes sense to have it either 1-based (but "all the way" as in this patch -- which could be backported to 3.6 I think) or zero-based (which is probably better to do in 4.0 for the reasons you mention).

So my suggestion would be: land this patch as it is in main, backport to 3.6, and followup with a change to zero-based in main only (4.0).

Opinions?

> For report #109491 the only code of your changes that is hit is in `BKE_curve_material_index_remove`. We have three materials and remove the last one so the index coming in is 2 while our values in `info->mat_nr` are 1-3. We are off by one and your change there fixes it nicely. Thats where I started as well, so I get your point. However, the report also mentions material slots moving up and down, so this would need more changes than the one in `BKE_curve_material_index_remove` and these involve material remapping, array permutation etc (see `BKE_object_material_remap`), which is not easily fixable with "arrays with holes". What I mean by that is that we dont actually have "values in `info->mat_nr` 1-3 " as you mentioned, but instead it is 0/2/3. Only if you specifically assign the first material to some letters, the mat_nr will change from 0 to 1 [which makes the whole situation more manageble, but cannot be required by the user] > > But I would consider leaving the rest, including the versioning, to a refactor. That way this fix could go into a future 3.6 release while 4.0 could have these zero-based. As much as I'd like to just have some short, backportable fix, leaving the rest of this patch out does not really fix it. Old files would still be broken. And since we already know the root of the issue, imho it only makes sense to have it either 1-based (but "all the way" as in this patch -- which could be backported to 3.6 I think) or zero-based (which is probably better to do in 4.0 for the reasons you mention). So my suggestion would be: land this patch as it is in main, backport to 3.6, and followup with a change to zero-based in main only (4.0). Opinions?

Philipp Oeser commented

2023-07-21 10:55:01 +02:00

Oh, and will merge with main again in a bit...

Philipp Oeser added 1 commit 2023-07-21 11:59:48 +02:00

Merge branch 'main' into 109491 c44968b5a6

Philipp Oeser commented

2023-07-21 12:01:18 +02:00

I merged main now, note there is a bit of confusion wrt versioning code see #108286 (comment)

I merged main now, note there is a bit of confusion wrt versioning code see https://projects.blender.org/blender/blender/pulls/108286#issuecomment-984126

Philipp Oeser added 2 commits 2023-07-21 14:52:23 +02:00

Merge branch 'main' into 109491 d4fb925be3

use "own" SUBVERSION bump d9452e03f4

Philipp Oeser commented

2023-07-21 14:53:24 +02:00

I merged main now, note there is a bit of confusion wrt versioning code see #108286 (comment)

merged again (above was fixed in 9ec018e1df)

> I merged main now, note there is a bit of confusion wrt versioning code see https://projects.blender.org/blender/blender/pulls/108286#issuecomment-984126 merged again (above was fixed in 9ec018e1df176713726f9a6db90c12b2e7dc5108)

Philipp Oeser referenced this pull request

2023-09-27 15:02:55 +02:00

Text objects: make CharInfo mat_nr zero-based #112954

Philipp Oeser referenced this issue from a commit

2023-09-28 11:47:00 +02:00

Text objects: make CharInfo mat_nr zero-based

Philipp Oeser commented

2023-09-28 11:57:22 +02:00

So this should be fixed in 4.0 with !112954.

Now we have to see if/what makes the most sense in terms of backporting.
Go with the zero-based solution in LTS as well?
Or take in the approach from this PR in LTS?
Do nothing in LTS?

So this should be fixed in 4.0 with !112954. Now we have to see if/what makes the most sense in terms of backporting. Go with the zero-based solution in LTS as well? Or take in the approach from this PR in LTS? Do nothing in LTS?

Hans Goudey commented

2023-09-28 14:48:20 +02:00

I guess the zero based indexing seems easiest to reason about, and best for forward compatibility.

Philipp Oeser commented

2023-09-28 14:50:49 +02:00

I guess the zero based indexing seems easiest to reason about, and best for forward compatibility.

OK, will close this PR then and make one that resembles !112954 for the LTSses

> I guess the zero based indexing seems easiest to reason about, and best for forward compatibility. OK, will close this PR then and make one that resembles !112954 for the LTSses

Philipp Oeser closed this pull request

2023-09-28 14:50:51 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #109491: Erratic behavior of materials for text objects #109746

Pull request closed