Page MenuHome

Clean up use of magic numbers in Double Edge Mask to defines instead
Needs ReviewPublic

Authored by Ted Schundler (tschundler) on Aug 21 2016, 11:27 AM.

Details

Summary

I was trying to fix some bugs in this code and found is tricky to read. I think it could use a large refactor to avoid so much copy-pasted content and improve legibility. But as a start, I did this, using defines to set up the magic numbers used, and to not use two separate references to the same block of memory. (*res and *lres are the same set of data, cast as different types)

Diff Detail

Repository
rB Blender

Event Timeline

Ted Schundler (tschundler) retitled this revision from to Clean up use of magic numbers in Double Edge Mask to defines instead.
Ted Schundler (tschundler) updated this object.
Ted Schundler (tschundler) set the repository for this revision to rB Blender.

let me know if the reviewer would like a sample test .blend file. Before and after this change, there is no difference in operation, just no more magic number use in this file.

Using proper constant names is surely good, but i'm not sure about lres/res change. It doesn't really make things easier to follow, and even if it's the same block, semantically code in some places easier to follow with more clear separation..

source/blender/compositor/operations/COM_DoubleEdgeMaskOperation.cpp
33

Can this constant be less cryptic (as in, avoid manual bitfield here)?

1006

As far as i can follow, lres was supposed to be something more like a runtime flag storage, res was supposed to be an actual node output.

From such point of view there are following issues here:

  • Instead of using proper intensity value (which is either 1 or 0 depending on where you are) you are now using some rather cryptic constant for that.
  • Such change kind of violates previous flow of if(flags == something) { result = something_else; } . Not saying original naming was really good, but new approach does not sound much better to me.
Ted Schundler (tschundler) edited edge metadata.

reverting to reparate lres & res buffer designation

Not treating 0.0/1.0 as named constants