Page MenuHome

Moving mesh_evaluate.c to C++
AbandonedPublic

Authored by Jagannadhan Ravi (easythrees) on Jun 23 2021, 11:56 PM.

Details

Summary

In researching some optimizations for normal generation, it's easier to implement in C++ than C. Unit tests pass on my end.

Diff Detail

Repository
rB Blender

Event Timeline

Jagannadhan Ravi (easythrees) requested review of this revision.Jun 23 2021, 11:56 PM
Jagannadhan Ravi (easythrees) created this revision.
Hans Goudey (HooglyBoogly) requested changes to this revision.Jun 24 2021, 7:59 PM

First of all, +1 to the idea of moving this file. I think the code could be much more readable with future changes to use Span, Array, etc.
And parallelization with parallel_foris usually much easier to read. But I think the approach of this patch (only doing minimal changes) is right (for now anyway).

  • Part of the patch doesn't apply cleanly anymore using arc patch --nobranch --nocommit D11688
  • Clang format should be applied to the file, it looks there are some formatting mistakes and too-long lines. (I really recommend setting up the editor to do this automatically!)
  • Personally I would replace BLI_SMALLSTACK_DECLARE with blender::Vector with an inline buffer, like Vector<short *, 32> clnors; rather than adding a cast in the header.
  • This patch could/should also change things that clang tidy cares about: NULL -> nullptr, removing typedef
This revision now requires changes to proceed.Jun 24 2021, 7:59 PM

Adding you as a reviewer Campbell because you've been working in this area recently and you should be aware of the patch probably. I don't think you'll need to check the changes in detail though, I'm happy to do that.

removed typedefs as well now

Hans Goudey (HooglyBoogly) requested changes to this revision.Jun 29 2021, 3:31 AM

I suggest looking over the diff after you submit it, because there are some large changes here (moving and copying of functions) that aren't necessary to move this file to C++.

source/blender/blenkernel/intern/mesh.c
1291 ↗(On Diff #38732)

Why is this new code being added here?

source/blender/blenkernel/intern/mesh_tessellate.c
157 ↗(On Diff #38732)

Why does this patch move this section from mesh_tessellate.c to mesh_evaluate.cc

That doesn't seem necessary.

This revision now requires changes to proceed.Jun 29 2021, 3:31 AM
source/blender/blenkernel/intern/mesh.c
1291 ↗(On Diff #38732)

This is where it was in master, no? I had to finish the merge manually. I'll remove it.

source/blender/blenkernel/intern/mesh_tessellate.c
157 ↗(On Diff #38732)

I thought this was where it was in master, I'll move it.

Hi, please accept my apologies for the inconvenience, but something's messed up with git on my end. I've done a fresh grab and redone these changes and will put them up in a new review. Again, please accept my sincerest apologies for the inconvenience.