Details
- Reviewers
Steve (Cruentus_Nex) - Maniphest Tasks
- T38317: Randomize (vertices)
Diff Detail
Event Timeline
Hi, yep - looks generally fine, but there are some details - noted inline.
| source/blender/editors/mesh/editmesh_tools.c | ||
|---|---|---|
| 4944 | the point of using ED_transverts*** is so it doesn't have to be spesific to mesh data. so this could be moved next to view3d snapping code which also uses ED_transverts | |
| 4969 | *picky* odd formatting, compared to other uses in blender. | |
| source/blender/editors/util/ed_transverts.c | ||
| 481 | fine for test but we have some better noise functions. | |
Changes:
Added Frequency, Amplitude, and Axis Limiter mods variables. The user can now select to constrain the randomization by axis and subsequently alter the depth and frequency of the noise. The randomization function is now using BLI_rand() instead of rand. I don't know if there's a better option, but I'm sure I will shortly. Moved the code that links into the UI over with the view3D code. The randomizer can be called on any object that has an edit mode. It doesn't really do anything to meta objects. Also trying to use this on a text object right now will crash blender, so I still have some kinks to work out, but it's functional for the most part. Wasn't sure how to keybind it so it's on CTRL + ALT + N. I thought it might be a good idea to integrate it into the 'W' menu, but I don't know how.
Generally good, comments inline.
| source/blender/editors/space_view3d/view3d_snap.c | ||
|---|---|---|
| 727 ↗ | (On Diff #940) | A bit misleading since this is a view3d operator, but it doesnt really require a 3d view, see: object_warp_verts_poll, this poll function could be made generic and apart of ED_transverts |
| 761 ↗ | (On Diff #940) | This could be an enum-flag, so you could select X and Y, or Y and Z for example. See: RNA_def_enum_flag use in editmesh_tools.c. Then ED_transverts_randomize would take one axis arg. |
| 783 ↗ | (On Diff #940) | use lowecase naming for properties. |
The axes are now an enum_flag so they can be selected and combined.
Renamed the poll function ED_transverts_obedit_poll and moved it into ED_transverts.
Replaced the poll function for warp functions with above.
Changed property names to lower case.
See comments inline.
| source/blender/editors/space_view3d/view3d_snap.c | ||
|---|---|---|
| 731 ↗ | (On Diff #961) | It would be better to do: |
| 743 ↗ | (On Diff #961) | Instead of repeating the values for each of the axis combinations here, use the enum defines you've made above. For example, this line becomes: Also, if you intend for the other options to be available, you'll need to define these here too. |
| 762 ↗ | (On Diff #961) | This whole block of if's can be simplified by simply doing something like: |
| 781 ↗ | (On Diff #961) | Perhaps "Apply random offsets to vertex locations" might be more descriptive? |
| 789 ↗ | (On Diff #961) | It's better to fully spell out the names of the identifiers here instead of abbreviating them. So, "amplitude" instead of "amp", and the same for frequency I guess. |
| 790 ↗ | (On Diff #961) | Typo - "Effect ever nth vertex" ;) Personally, I'd probably name this "step_size" or something similar instead. Judging by the name, I'd have thought that this controls the frequency of the underlying noise function instead. |
| source/blender/editors/util/ed_transverts.c | ||
| 480 | Whitespace nitpick - There should be a single space between "for" and the opening paren (i.e. "for (..." instead of "for(...") and the same goes for the end of the line here (i.e. "freq) {" instead of "freq){") | |
| 482 | IMO it's probably better that you don't try to squeeze everything onto a single line here using a ternary operator and assignment statements nested inside this. AFAIK, there's no real performance gain from doing this, while the non-standard syntax can momentarily trip people up (mainly by overlooking where exactly the assignments are happening). | |
| source/blender/editors/space_view3d/view3d_snap.c | ||
|---|---|---|
| 734 ↗ | (On Diff #1021) | Why define these? just (or) | any of the 3 flags. This is what enum flags are for. |
Note that this won't make it for the up-coming 2.70 release.
but by all means get it ready for inclusion after release, when we will have time to do final review and commit it.
| source/blender/editors/space_view3d/view3d_snap.c | ||
|---|---|---|
| 740 ↗ | (On Diff #1030) | Looks like these do nothing. |