samdauwe / BabylonCpp

A port of Babylon.js to C++
Apache License 2.0
284 stars 37 forks source link

Hunt segfaults #53

Closed pthom closed 4 years ago

pthom commented 4 years ago

Hello,

This is a PR where I corrected some errors in the examples and in the general code.

Status on the examples failures:


Based on this modifications, I have a few advices:

Custom sorts

Use BABYLON::stl°util::sort_js_style when porting a custom sort function

std::sort needs a predicate (i.e a function that returns a bool) javascript sort needs a function that return -1 if a<b, 1 if a>b and 0 if a==b However, in C++ both -1 and 1 are casted to true ! BABYLON::stl_util::sort_js_style is a new function that accepts a js style custom sort function. I checked all the calls to std::sort in order to make sure whether they needed to use this new function or not

Issues with lambda captures:

I found lots of issues with lambda captures. When a lambda function can outlive the current scope, all its captures by reference will be dead when the lambda tries to use them.

So, here are some tips:


Unit test around the bug in CartoonsAnimationScene:

If you look at the new test mesh_test.cpp, you will see a line that is commented out:

auto ss = sphereLight->clone("");

It reproduces the reason that makes this example fail. I tried to solve it, but it is deep in the framework. Coud you try to solve this test?


Cheers, Pascal

pthom commented 4 years ago

Concerning the memory and lifetime issues I think a global check with valgrind is more than needed : the default checker of valgrind is memcheck and it shows lots of issues that need to be addressed

samdauwe commented 4 years ago

Hello Pascal,

Thanks for those bug fixes! Looks like more and more examples starting to work :)

That sort helper function is really helpful, porting the sort functions from JS to C++ is often confusing.

Lambda captures are indeed a point of attention. A good rule would be to list each variable in the capture list that needs to be captured and double check the scope. There are so many lambdas in de codebase that I will double check them for each file I update, so this will take some time.

Regarding the CartoonsAnimationsScene, I already could fix some of the issues:

The example is not working yet, it needs further debugging, I will continue tomorrow and enable the unit test when I am further with resolving the issues

Running valgrind is a good advice, something to do next :)

Cheers, Sam

samdauwe commented 4 years ago

Hello Pascal,

the CartoonAnimationsScene example is now working on Linux. I will test the Windows version later. At the end of the whole animation the camera turns to an angle that is different from the BabylonJS version, I will look into this later.

I also enabled the unit test you created (with some modifications). Furthermore, there was an issue with the picking info I also fixed.

Going to continue fixing other issues.

Cheers, Sam