- Amsterdam, Netherlands
- https://cessen.com
-
Animator, rigger, and software developer. Currently working at the Blender Institute as a developer on Blender's animation system.
Been using Blender since 1998, and worked on Big Buck Bunny and Sintel (two of Blender's open movie projects).
- Joined on
2003-03-21 -
-
Same comment as before, about preferring includes of the relevant header(s) to make it explicit where things come from. Although see this in other places as well, it appears this is a common pattern in Blender's code base. So maybe never mind.
When we were looking through this in person, I noted that we could eliminate this padding by arranging the fields from largest to smallest. However, looking at other DNA structs in this PR, it looks the strategy is to ensure a multiple-of-8-bytes size anyway, which I assume is a general pattern/requirement in DNA (which totally makes sense).
It looks like this is defined in DNA_userdef_types.h. I'd be more comfortable just including that instead to make it explicit where ThemeWireColor is coming from. I guess the concern here is bloating build times?
These should be disabled if there is no active_bcoll. (Just needs to be added to poll.)
These should be disabled if there is no active_bcoll. (Just needs to be added to poll.)
How does it sound to both of you? :)
Sounds good! I think starting a topic on devtalk, and pointing to PRs of alternatives with builds available, would be a good way to go. But I'm happy to…
Of course smoothing has performance impact, but it's not much compared to the arctangent used by the slope based factor.
Yeah, that's a good point. Having said that, I was playing around…
This looks ready to land to me. I still have some minor nits, but none of them are functional or user-facing, and can be easily addressed later.
I did write it in the space's description though.
Ah! Sorry! I somehow missed that.
The description still feels a bit vague to me. In particular, it's not clear to me what "compensate…
Aside from adding comments explaining the Luminance Compensation * transforms, I think this is ready to land.
Just adding a comment sounds good to me. @Eary If you also want to rename it to "Lower Guard Rail" I think that would further provide clarity, but I don't feel strongly as long as there's a…
What's the rationale for having separate options for location, rotation, and scale vs just having a single "transforms" option?
It looks like all of these keyframes.add_paths(...) in the conditionals are already run unconditionally on lines 314-319. Were those lines meant to be removed?
Also, I notice that in the "After" image, it looks like there's a now a slight discontinuity on the outside edges, which isn't in the "Before". Is that just an artifact in the visualization…
Thanks for putting together this awesome blend file! Really helps.
- Unless there's a demonstrable quality difference between the methods, I think we should go with the fastest executing one,…
This basically looks ready to land to me. Just curious about that one constant.
What is the rationale for this constant? Intuitively I would expect it to be something like 1.0 or 1.5, to only merge sub-pixel details.
@nathanvegdahl Wasn't the case for it made already blender/blender#106355 (comment) ?
The hue skewing issue demonstrated there is caused by…