Page MenuHome

New BLI Function: projmat_from_window_region
ClosedPublic

Authored by Germano Cavalcante (mano-wii) on Mon, Aug 5, 4:38 AM.

Details

Summary

I have spent some time working on this function to use in a project that I am working on locally.

To test the function I used it in ED_view3d_clipping_calc and since it is more efficient than the current solution, I thought it best to leave it that way.

(In addition to efficiency, having this function in BLI can simplify reviewing future patches I'm working on.)

Diff Detail

Repository
rB Blender

Event Timeline

  • Accidentally removed
  • BoundBox is optional
Brecht Van Lommel (brecht) requested changes to this revision.EditedMon, Aug 5, 10:38 AM

Please explain with a comment in the code what this function does, it's not immediately obvious to me. What is different about it than the dozens of other similar functions we have.

This revision now requires changes to proceed.Mon, Aug 5, 10:38 AM
  • Add comment explaining what the function does
  • Remove part of the comment
  • Rename projmat_from_window_region to projmat_for_window_subset
  • Use new matrices created to unproject.
Brecht Van Lommel (brecht) requested changes to this revision.Mon, Aug 5, 5:05 PM

As explained in private discussion, I think this makes ED_view3d_clipping_calc more complicated.

Previously most of the logic was shared with ED_view3d_unproject, now there's a whole bunch of extra code that duplicates its implementation details.

And if projmat_for_window_subset is intended for clipping objects outside the selection region, then I don't see why we need a new function for that if ED_view3d_clipping_calc can already return the required clipping planes.

This revision now requires changes to proceed.Mon, Aug 5, 5:05 PM
Campbell Barton (campbellbarton) requested changes to this revision.EditedMon, Aug 5, 5:05 PM

Generally having a projmat_for_window_subset function LGTM & seems useful, comments inline

source/blender/blenlib/BLI_math_geom.h
638

subset name seems a bit odd, could call projmat_from_subregion or projmat_calc_subregion

639

Using short here happens to be convenient but isn't so good for a general API.

source/blender/blenlib/intern/math_geom.c
4502–4503

These relate to x/y, calling a/b isn't that helpful.

Germano Cavalcante (mano-wii) marked 3 inline comments as done.Mon, Aug 5, 5:58 PM
Germano Cavalcante (mano-wii) updated this revision to Diff 16851.
  • Remove all changes in ED_view3d_clipping_calc;
  • Rename to projmat_from_subregion;
  • Use int instead short;
  • Use more descriptive names for the variables.
This revision is now accepted and ready to land.Mon, Aug 5, 6:54 PM