Refactor RenderScale #920
Refactor RenderScale #920
Conversation
- Moved declaration to its own file. - Removed inheritance from OfxPointD so implementation could be changed. - Made data members private to hide implementation and created helper methods to capture common use patterns. - Change TrackerNodeInteract::selectedMarkerScale to a double since it didn't really need to be a RenderScale. - Added named constant for RenderScale with x & y set to 1.
Engine/RenderScale.cpp
Outdated
| RenderScale | ||
| RenderScale::fromMipmapLevel(unsigned int mipmapLevel) | ||
| { | ||
| return RenderScale(mipmapLevel); |
Engine/RenderScale.cpp
Outdated
| unsigned int | ||
| RenderScale::toMipmapLevel() const | ||
| { | ||
| return mipmapLevel; |
Engine/RenderScale.cpp
Outdated
| OfxPointD | ||
| RenderScale::toOfxPointD() const | ||
| { | ||
| const double scale = Image::getScaleFromMipMapLevel(mipmapLevel); |
There was a problem hiding this comment.
inline and move function to RenderScale where it fits better than Image?
There was a problem hiding this comment.
done. Also removed the function from Image since this change eliminates most of the callers.
Engine/RenderScale.cpp
Outdated
| // static | ||
| const RenderScale RenderScale::identity; | ||
|
|
||
| RenderScale::RenderScale(unsigned int mipmapLevel_) |
| constexpr RenderScale() = default; | ||
|
|
||
| // Named constant where x & y scale are both 1. (i.e the identity scale factor in both directions.) | ||
| static const RenderScale identity; |
There was a problem hiding this comment.
does that mean that this constant will never be inlined? if this is the case, why not use the default constructor instead of this named constant? Or could this be constexpr instead?
There was a problem hiding this comment.
Since most RenderScale parameters are const RenderScale& the inlining doesn't really matter. This static var would always be a constant address that would get passed to the function whereas the default constructor would just use some stack space with a constant value. No real perf or code size difference here. I mainly added this constant to help with readability since it appeared that some code was trying to use names like scaleOne or explicitly specifying a scale of 1 to a constructor to signal identity. I'm fine with just using the default constructor everywhere if you'd like.
I tried making this constexpr but ran into problems. I believe I need to have C++17 semantics to get that to work. FWIW I'd love to upgrade to C++20 (but could settle for 17). I suspect that's a discussion for another pull request though.
Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:
PR Description
What type of PR is this? (Check one of the boxes below)
What does this pull request do?
This change refactors RenderScale so that much of the code that uses it is much simpler. It also makes it much more explicit that Natron does not actually allow/support the x & y scale to be different from each other. Functionally this should be equivalent to the existing code and no new functionality was added.
Have you tested your changes (if applicable)? If so, how?
Yes. I've tested this locally with a few project files. The unit tests all pass locally and in the GitHub Actions.
Futher details of this pull request
This came about mainly from me trying to understand why mipmapLevels and RenderScales were being passed around to different APIs and sometimes both were passed together. There also appeared to be more conversions to/from mipmapLevel then actual use of the doubles so it seemed to make sense to just convert the underlying representation to the mipmapLevel. I suspect further simplifications can be made in follow up changes and some usage of RenderScale can go away or be replaced with the equivalent mipmapLevel parameter. This change was just intended to leave the existing abstraction in place, but make the API more explicit and constrained.