LGTM

Upstream for quadriflow is pretty much dead.
We could probably have these changes upstreamed, but I'm not sure if we should wait for this.

In T80194#1048915, @Hans Goudey (HooglyBoogly) wrote:

Could the baking settings be moved to the modifier? That way you could get editable strokes for only one collection / object? I just tested and it seems this isn't possible right now.

In T80194#1048812, @Brecht Van Lommel (brecht) wrote:

From looking at the code, it seems that the modifier actually outputs the geometry, and further modifiers can be applied on top of it.

Correct.

In T80194#1048778, @Hans Goudey (HooglyBoogly) wrote:

In D8758#234971, @Brecht Van Lommel (brecht) wrote:

The intent as I understand it is to get a basic version of Line Art ready for master, as a base to build further improvemnts on. We could support both as a modifier or rendering feature in the future. But for the basic version please pick one and then we can ensure the UI and implementation for that works well.

In D8758#234869, @Brecht Van Lommel (brecht) wrote:

This would mean you implement this feature as a regular grease pencil modifier, generating strokes. Depsgraph integration shouldn't be very difficult, probably it's just a matter of implementing a modifier in a way that is similar to existing modifiers and this might require no direct changes to the depsgraph.

The thresholds and angle splitting are artistic choices as far as I can tell. The line types I guess could be useful as global overrides since they are also in the modifier, though I'm not sure how useful it is in practice.

To reduce any confusion about the modifier part of things I'll explain the way we came here a bit.

In T81452#1046221, @Campbell Barton (campbellbarton) wrote:

I get where you're coming from, my main concern with this is - in practice it means extending existing comments would be tedious - or developers would forget to change the comment style & we'd end up with a mix of both... having to make clean-up commits to enforce the rule.

I've already had to fix old comments that didn't follow our current guidelines in cleanup commits, so we already have issues like this.

I'll just throw in my two cents:

@Daniel Salazar (zanqdo) why is this moved to known issues when we have a fix in review for it?

Updated with latest feedback.

Also, should we back port this?

Updated with feedback

@Hans Goudey (HooglyBoogly) thanks for fixing this!

Ok, this issue doesn't happen with "Undo Legacy" activated (Preferences -> Experimental -> Undo Legacy).

Updated with latest feedback.

Updated code to latest master and incorporated given feedback.

The test should now pass on windows. Thanks for pointing this out!

Use ctest -VV -R libmv_vector_test if you want more verbose output on why it failed.

You have to provide more info than that.

@Brecht Van Lommel (brecht) so how are we going to do this? Are we going to do the remove_cc_flag route?
To me it does makes sense to have the contract=off flag for GCC as it is the default with both clang and the intel compiler it seems (I don't know if it is the same with windows VS compiler)

Updated with the latest feedback.

In D8968#228904, @Sergey Sharybin (sergey) wrote:

I am still waiting for the inlined comment about template alias to be addressed.

At least in the example file, this is the case.

I've updated the documentation to reflect the actual behavior of how rigidbodies becomes activated.

I think it looks fine code wise still.

LGTM

I thought we had the bending springs working when I helped you implement them a while back?

I agree with Sergey's comments

This does look fine to me.

@Sybren A. Stüvel (sybren) not yet, I talked to Brecht and Sergey about it and it seems like I should probably make a bigger proposal and design about this.

The only thing I have to add is that we should probably port this to other parts of blender that uses the same logic.
I there are a few other operators (like the decimate keyframes one) that does the same thing.

I think this looks good to me as well.

In D8968#223707, @Brecht Van Lommel (brecht) wrote:

It seems the move construct must use noexcept:
https://stackoverflow.com/questions/8001823/how-to-enforce-move-semantics-when-a-vector-grows

Perhaps we can use the remove_cc_flag macro and simply do remove_cc_flag("-ffp-contract=off") for cycles?

In D8968#223537, @Brecht Van Lommel (brecht) wrote:

If you add a move constructor to Foo, then the number of constructor/destructor calls should no longer be STL implementation-dependent, and the number of calls should go back to what it was before.

Foo(Foo&& other) = default;

I don't spot anything bad/wrong either. LGTM

In D9047#223543, @Brecht Van Lommel (brecht) wrote:

Adding it to the global compiler flags here may disable FMA in the Cycles kernel with optimized compile flags (and potentially other code), where we do actually want to keep it enabled.

Cycles optimization flags will be added after the PLATFORM_CFLAGS, so if the order -ffp-contract=off -match=native still enables FMA it's fine. Can you check if that is the case?

Sure, but my point is: number of re-allocations will depend on how exactly STL grows the capacity. GCC and MSVC are using different strategies, i.e. It might not be an immediate issue here, but relying on such match is like burring a dead cat into the code.

Hmm, perhaps we should just remove this test then?
If we are using the std::vector class as base, I don't think testing this is that useful?