The purpose of this patch is to add the option to create a color
attribute paint slot from the canvas selector when in material mode.
See the discussion here: T97346
| Add Image Paint Slot | Add Color Attribute Paint Slot |
 |  |
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- canvas_ui (branched from master)
- Build Status
Buildable 21931 Build 21931: arc lint + arc unit
Event Timeline
Comment Actions
Small tweaks
- Use expanded buttons consistent with D14785.
- De-duplicate one line of code.
- Tweak tooltips.
Comment Actions
- Make includes explicit
Only for includes relevant to this patch (a cleanup can fix others.)
Comment Actions
For creating new attributes or image textures this works fine. I couldn't test it much further because of regular crashes in Eevee. The crash seems to happen in general when using the experimental textture painting in sculpt mode though. I'll investigate that first
Comment Actions
Thanks for looking into this. In my review I'm mainly looking at cleanup, but also, unless I'm missing something, it looks like there's a fair amount of non-functional refactoring that should be included separately from this change.
| source/blender/editors/sculpt_paint/paint_image_proj.c | ||
|---|---|---|
| 6418–6426 | const looks misplaced here. Really, this sort of cleanup shouldn't be in a patch with functional changes though. If you split out things like this and add me as a reviewer, I'd be happy to take a look. but including it in patches like this makes it harder to find the real changes and do a proper review. | |
| 6435–6436 | These translation markers shouldn't be necessary-- any enum exposed to RNA is meant to be scanned by the translation system already. Either way, it shouldn't be included in this patch, since it's a separate concern. | |
| 6450–6457 | It doesn't look like there's any need to reorder these, is there? | |
| 6676 | It's not clear to me why this handling of materials is changing, I thought this patch was just meant to add something to this popover. It actually looks like there are a fair amount of non-functional changes to the handling of materials elsewhere in this patch too. | |
| 6823–6833 | These enum arrays shouldn't be duplicated again-- should be possible to reuse their definitions (maybe that's in another patch? | |
| 6830–6859 | Please avoid doing cleanup in a patch with functional changes wherever possible. For example, the order of "color" property definition doesn't have to change here. | |
| source/blender/editors/sculpt_paint/paint_image_proj.c | ||
|---|---|---|
| 6676 | I will make the changes to material handling in a separate patch. When testing, I noticed that the invoke method creates a material. This was done to patch a bug, but creating a new material in the invoke method is not the ideal solution as the material is added even if the operation is canceled before execution. So, I fixed the bug in a different way. Also, creating the material and setting the default color in invoke makes the calls to get_or_create_current_material and proj_paint_default_color in exec redundant. | |
| 6823–6833 | I saw that was a part of D14733. | |
| 6830–6859 | I reordered "color" since it is a property shared by image paint slots and color attribute paint slots. Either it has to move, or I have to revert the comments that describe the three sections of the properties. | |
| 6836 | I rename the text from "Type" to "Material Layer Type" since the new "slot_type" property introduces ambiguity. Since I changed the label, I also fix the description since it was obviously a copy paste error. | |
Comment Actions
Thanks for splitting out the refactoring, the patch is much clearer now.
| source/blender/editors/sculpt_paint/paint_image_proj.c | ||
|---|---|---|
| 6823–6833 | I just committed that change separately so we can move on from that :) | |
| source/blender/editors/sculpt_paint/paint_image_proj.c | ||
|---|---|---|
| 6676 | Yeah, it probably makes more sense for the exec method to create a material. IIRC generally "invoke" just meant to set up an operator based on user input, not actually do anything. | |