This brush deletes displacement information of the Multires Modifier,
resetting the mesh to the subdivision limit surface.
This can be use to easily delete parts of the sculpt or to fix
reprojection artifacts after applying a shrinkwrap.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- multires-displacement-eraser (branched from master)
- Build Status
Buildable 9486 Build 9486: arc lint + arc unit
Event Timeline
| source/blender/blenkernel/intern/subdiv_ccg.c | ||
|---|---|---|
| 1936 | get is usually accessor, which is expected to be cheap (at least in Blender). This function is not an accessor, it does actual evaluation, so call it eval. Keep naming consistent with other related areas. Here you've named function BKE_subdiv_ccg_limit_point_normal_get and it basically pass-throughs coordinate to BKE_subdiv_eval_limit_point_and_normal. Call this function BKE_subdiv_ccg_eval_limit_point_and_normal. But also, seems you never use normal, so it should be BKE_subdiv_ccg_eval_limit_point. | |
| 1941–1963 | One function -- one task. | |
| 1943–1945 | Why this explicit cast to float? Please read up of how type conversion works in C/C++. Order u first, v second. | |
| 1961–1962 | How's this different from grid_u and grid_v? | |
| 1965–1971 | This doesn't make sense to me. subdiv_ccg->has_normal denotes whether there is a CCG elements for normals stored in the SubdivCCG. What are you trying to do here? | |
| source/blender/editors/sculpt_paint/sculpt.c | ||
| 2952–2954 | Please pay attention to feedback in other patch review and apply feedback in all future work. https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Variable_Scope | |
| 2975–2977 | Do not evaluate normal if it's not needed: it is not coming for free. | |
Comment Actions
Usually the code that has style and quality problems is directly copied from another functions of the sculpt mode code from 2.80 (in this case, the lines with explicit casts are in subdiv_ccg_eval_regular_grid and the main code of the brush is the same as the regular draw brush). If this is a problem, then it would make sense to create a separate task to list all the cleanups that need to be done to those files so all the code of the module is consistent.
Comment Actions
Well, first of all, you can always poke me in any communications channels saying "hey, i think I've found something stupid in your code ;)" and we quickly address it without going into "burocracy" of creating tasks. Keeping quality, easy to follow module is a team activity.
And second, I am not convinced it is better to "spread" "stupid" things from the module to the newly added functionality, especially when it doesn't take effort to do things "properly".
On more of the review topic, please add const and I think it'll be good to go.
| source/blender/blenkernel/intern/subdiv_ccg.c | ||
|---|---|---|
| 1942 | Think this can be const. | |
| 1951 | const | |
| 1953 | const | |
Comment Actions
@Pablo Dobarro (pablodp606) I went ahead and submitted D8552. Easier than dealing with tasks ;)
| source/blender/blenkernel/intern/subdiv_ccg.c | ||
|---|---|---|
| 1942 | This one creates a warning when passing it to BKE_subdiv_face_ptex_offset_get | |
| source/blender/blenkernel/intern/subdiv_ccg.c | ||
|---|---|---|
| 1942 | Eh, will rename it to BKE_subdiv_face_ptex_offset_ensure. | |