Remove global G_PICKSEL, pass object drawing context.
Needs RevisionPublic
Actions

Authored by Campbell Barton (campbellbarton) on Jan 14 2014, 6:17 PM.

Details

Reviewers

Summary

This patch makes the following changes

Replace (G.f & G_PICKSEL) with (dflag & DRAW_PICKING), which is already being used in places.
Merge {dt, dflag} into a struct and pass it around as (const ObjectDrawCtx *) because this pair of args was already being passed around everywhere - (we could have ob_wire_col passed too but I prefer not for now since it becomes unclear which functions need colors).
split draw_glsl_material into draw_glsl_material_ex which takes ObjectDrawCtx and draw_glsl_material which uses the v3d->draw_type.

Generally it's big +1 to get rid of global state. Some minor notes, mainly about structure initialization and naming in some cases.

Didn't apply patch yet tho, looking into other bugs atm.

source/blender/blenkernel/BKE_global.h
108	Use /**/ comments for C code.
source/blender/editors/space_view3d/drawarmature.c
985	Call it `draw_flag`. `dflag` is really cryptic and there're more bitfields here which confuses even more. Think of those who read the code first time troubleshooting the stuff and be more verbose!
2346	Don't feel positive about this. Would give issues when `ObjectDrawCtx` is extended or it's members are reshuffled. I would go with initialization to {0} and filling the fields explicitly with `dt_ctx.foo = bar`.
2415	Same as above.
source/blender/editors/space_view3d/drawmesh.c
945	And here the things are really creepy. We've got `dflags` which i believe stands to `draw flags` and `draw flags`. Naming, need better naming..

Nothing to add really besides what Sergey already wrote.

source/blender/editors/space_view3d/drawmesh.c
945	I guess the existing draw_flags can be renamed to mesh_draw_flags as they are specifically for meshes. Or folder into the drawing context somehow, not sure how far we want to take that concept.

Firstly I'm in no rush to apply this patch, but eventually drawobject.c could have a lot of improvements.

I'd like to know if we're going to merge the viewport-fx branch, since this could conflict.

source/blender/editors/space_view3d/drawarmature.c
2346	Valid concern, think it could be mitigated, Use enum types - so mismatch will warn. But for this type IMHO its ok since It will stay small, no intention to start adding scene, v3d, rv3d.... etc. Also, if you define then assign it means you can't declare `const`
source/blender/editors/space_view3d/drawobject.c
6646	I don't like this but didnt see a better alternative, dflag_ I wanted to pass `dt_ctx` in as const, but it turns out `draw_object` changes it based on own internal logic. Ill check on making it work different though.

source/blender/editors/space_view3d/drawarmature.c
2346	For the DupliContext in D189, which is also const, i've been using an init function that returns DupliContext by value. This may be over the top in this case, just thought i'd mention it. We seem to be afraid of return-by-value in Blender code - understandably given the potential copy overhead, but as long as it's static functions and/or inlined this wouldn't be a problem. It's basically what happens in C++ constructors.
source/blender/editors/space_view3d/drawmesh.c
970	I've seen the use of const [bool, int, ...] in a number of places. This is fine of course and might help indicate a shortcut variable, but i'm slightly worried it may become a habit and we end up with inconsistent const keyword everywhere. Technically nothing is really gained in performance by adding const here: As long as the variable is used in a single compilation unit and not passed by reference into other functions the compiler can easily detect that it's not touched and use it as const anyway. http://stackoverflow.com/questions/10747927/should-i-use-const-for-local-variables-for-better-code-optimization
1086	Similar remark as above: `draw_flags` is very much like a local variable, as long as you don't pass it on by reference the const attribute is not needed.