material.pop breaks mt->mat_nr #28111

New Issue

Dalai Felinto · 2011-07-29T09:46:22+02:00

Dalai Felinto commented

2011-07-29 09:46:22 +02:00

%%%code comment: After the material is removed the old mat_nr assigned to the faces is not remapped. (in material.c::material_pop_id())

In the attached file run the script:

import bpy

ob=bpy.context.object
me=ob.data

me.materials.pop(1)

now the third face (which has a valid material) no longer point to a valid material.

at the same time the second face (which should be re-set to material 1) has mat3 (since mat_nr was '1' and still is - but now mat_nt refers to mat3 and not mat2 as it was).

This will mess up with the materials of the faces.%%%

%%%code comment: After the material is removed the old mat_nr assigned to the faces is not remapped. (in material.c::material_pop_id()) In the attached file run the script: **** import bpy ob=bpy.context.object me=ob.data me.materials.pop(1) # now the third face (which has a valid material) no longer point to a valid material. - at the same time the second face (which should be re-set to material 1) has mat3 (since mat_nr was '1' and still is - but now mat_nt refers to mat3 and not mat2 as it was). **** This will mess up with the materials of the faces.%%%

Dalai Felinto commented

2011-07-29 09:46:22 +02:00

Changed status to: 'Open'

Dalai Felinto commented

2011-07-29 09:51:27 +02:00

%%%It seems like a simple fix actually. something along: for MFace in mesh: if mat_nr > index mat_nr --;
I guess the material_append_id function is broken too.%%%

%%%It seems like a simple fix actually. something along: for MFace in mesh: if mat_nr > index mat_nr --; I guess the material_append_id function is broken too.%%%

Dalai Felinto commented

2011-07-29 10:55:42 +02:00

%%%attached a patch to fix this%%%

Dalai Felinto commented

2011-07-29 11:11:36 +02:00

%%%repeating what I said in the mailing list:
"It's a small and simple patch, but I'm checking for specific datablock types in the functions (i.e. if (GS(id->name) == ID_ME) ) and I'm not so confident this is the best way to go. Until now the functions are all dealing with the datablock type in a generic way (e.g. by using give_matarar_id(ID *id)).

An alternative (slightly more elegant) is to add a function give_mesh_id(ID *id) which returns a Mesh or null. Or even a "give_data_id(id, ID_ME)"."%%%

%%%repeating what I said in the mailing list: "It's a small and simple patch, but I'm checking for specific datablock types in the functions (i.e. if (GS(id->name) == ID_ME) ) and I'm not so confident this is the best way to go. Until now the functions are all dealing with the datablock type in a generic way (e.g. by using give_matarar_id(ID *id)). An alternative (slightly more elegant) is to add a function give_mesh_id(ID *id) which returns a Mesh or null. Or even a "give_data_id(id, ID_ME)"."%%%

Brecht Van Lommel commented

2011-07-29 13:40:26 +02:00

%%%The function material_pop_id duplicates code from object_remove_material_slot, which has code to do this mat_nr remapping. Ideally these function should share as much code as possible, but if you don't want to make too many changes, at least this part code be put in a common function.%%%

Campbell Barton commented

2011-07-30 04:25:01 +02:00

%%%Not sure if this is a bug, when writing exporters I had to add in support for out of range material values since this wuold happen from time to time, even though the UI tries to disallow it.

Theres also a case where you may want to rebuild material list without changing the face materials.

So if this behavior is changed I think it should be optional.%%%

%%%Not sure if this is a bug, when writing exporters I had to add in support for out of range material values since this wuold happen from time to time, even though the UI tries to disallow it. Theres also a case where you may want to rebuild material list without changing the face materials. So if this behavior is changed I think it should be optional.%%%

Dalai Felinto commented

2011-07-30 08:03:57 +02:00

%%%Campbell, for one to rebuild a material list you would need a 'replace' function, no? The materials.append takes no "index" parameter. (unless you remove all of them and append them in order. It seems a strange workflow though, even for a hack solution.

imho out of range materials is a bug to be addressed at read time, not a case to be reinforced again here.

Attached a matpop.diff patch for the problem following Brecht instructions (I did it early before your reply).
Note that the current behavior (which removes the material slot AND the material) really seems like a bug.

An alternative is to make the materials.pop() function to remove the material and keep the material slot (and skip material number remapping).%%%

%%%Campbell, for one to rebuild a material list you would need a 'replace' function, no? The materials.append takes no "index" parameter. (unless you remove all of them and append them in order. It seems a strange workflow though, even for a hack solution. imho out of range materials is a bug to be addressed at read time, not a case to be reinforced again here. Attached a matpop.diff patch for the problem following Brecht instructions (I did it early before your reply). Note that the current behavior (which removes the material slot AND the material) really seems like a bug. An alternative is to make the materials.pop() function to remove the material and keep the material slot (and skip material number remapping).%%%

Dalai Felinto commented

2011-07-31 13:15:25 +02:00

%%%fixed on rev. 38879.
from the log:

create a new parameter for materials.pop() to not remove material slot.
this way the mat_nr is still the old one.

for the default behaviour we now have material remapping (i.e. data_delete_material_index_id(id, index)).
This new function is brought from the material_slot remove function.

While running tests I got some:

Error: Not freed memory blocks: 2
Data from OB len: 24 0x11cc26d88
Data from OB len: 4 0x11cc26de8

I couldn't reproduce though, so I take that the code is fine. If there is a problem I suspect it would in the code for: /* don't remove material slot, only clear it*/ (material.c:603).%%%

%%%fixed on rev. 38879. from the log: **** create a new parameter for materials.pop() to not remove material slot. this way the mat_nr is still the old one. for the default behaviour we now have material remapping (i.e. data_delete_material_index_id(id, index)). This new function is brought from the material_slot remove function. **** While running tests I got some: **** Error: Not freed memory blocks: 2 Data from OB len: 24 0x11cc26d88 Data from OB len: 4 0x11cc26de8 **** I couldn't reproduce though, so I take that the code is fine. If there is a problem I suspect it would in the code for: /* don't remove material slot, only clear it*/ (material.c:603).%%%

Dalai Felinto commented

2011-07-31 13:15:25 +02:00

Changed status from 'Open' to: 'Resolved'

Dalai Felinto closed this issue

2011-07-31 13:15:25 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

material.pop breaks mt->mat_nr #28111

now the third face (which has a valid material) no longer point to a valid material.