MADEAPPS / newton-dynamics

Newton Dynamics is an integrated solution for real time simulation of physics environments.
http://www.newtondynamics.com
Other
942 stars 182 forks source link

CSharp: Linux works - windows broken #285

Open perkinslr opened 2 years ago

perkinslr commented 2 years ago

So when my brother saw I had newton working with python via swig, he asked if it worked with CSharp. So I duplicated the newtonPy sub-project to newtonSharp, changed the swig options, removed the post-install steps, and told it to build. On linux, it "just worked". After manually compiling the .cs files (csc $(find . | grep cs$) -target:library -out:newton.dll), it even successfully ran a simple world test with no further intervention.

When he followed the same procedure on windows, both using msvc and using clang, it compiles, but then fails the linking step for the newtonSharp.dll with missing symbols. I'm including the traceback for the clang try, as it's easier to read than the msvc failure, but I can probably furnish that one too if you need it.

The odd thing is the python extension links fine. I know windows is odd about what symbols it exports vs what symbols it hides, but I don't know enough about windows development to begin to sort that out.

Any advice on how to proceed would be appreciated, and if it's not too much trouble, can you at least check if it works on your (presumably good) windows dev box? It would at least let me rule out a bad configuration in my windows VM.

lld-link: error: undefined symbol: __declspec(dllimport) public: static class ndVector ndVector::m_half
>>> referenced by D:\Users\IEUser\Unity\NewtonPhysics\newton-4.00\applications\toolsAndWrapers\newtonSharp
\newton_wrap.cxx:1518
>>>               applications/toolsAndWrapers/newtonSharp/CMakeFiles/newtonPy.dir/newton_wrap.cxx.obj:(CS
harp_ndVector_m_half_set)
>>> referenced by D:\Users\IEUser\Unity\NewtonPhysics\newton-4.00\applications\toolsAndWrapers\newtonSharp
\newton_wrap.cxx:1526
>>>               applications/toolsAndWrapers/newtonSharp/CMakeFiles/newtonPy.dir/newton_wrap.cxx.obj:(CS
harp_ndVector_m_half_get)
>>> referenced by D:\Users\IEUser\Unity\NewtonPhysics\newton-4.00\sdk\dCore\ndVectorSimd.h:371
>>>               applications/toolsAndWrapers/newtonSharp/CMakeFiles/newtonPy.dir/newton_wrap.cxx.obj:(pu
blic: class ndVector __cdecl ndVector::InvSqrt(void) const)

Also a bunch of missing symbols related to ndBigVector::...

JulioJerez commented 2 years ago

ok, I guess this is as good time as any to wrap c sharp. I added the cmake script by basically copying the python version and editing it.

it builds the project and compile a dll, but there are some warnings that are in fact error. stuff like C:\Development\newton-dynamics\newton-4.00\applications\toolsAndWrapers\newtonSharp......\sdk\dCore\ndMatrix.h(56) : Warning 503: Can't wrap 'operator []' unless renamed to a valid identifier. ....

could be potential error, because the can result to an incorrect function call, so they all have to be resolve by adding explicit renaming or disable warning. but you can try syncing and build int and tell me what you get, before moving on.

here is one tip. when you build your wrapper, in c make, you can set NEWTON_BUILD_SHARED_LIBS off this way you get just one dll, for you project.

dlls are good for debugging but, in many cases if these are wrapper for different languages, it is nice that the entire thing is just one lib and one dll. at least for me, I like it better that way.

JulioJerez commented 2 years ago

When he followed the same procedure on windows, both using msvc and using clang, it compiles, but then fails the linking step for the newtonSharp.dll with missing symbols. I'm including the traceback for the clang try, as it's easier to read than the msvc failure, but I can probably furnish that one too if you need it

yes but let us make official, if you sync, I added few fixed to the python script, mainly those warnings, plus I added the solver plugins.

I also added the start of the c sharp script, so whatever modification you'd make, you can add it and submit a pull request.

JulioJerez commented 2 years ago

ok, I now added the proper renaming for operator overload method in some of the math classes. this applies to almost all wrappers. for example, if you use a vector in cpp, you and add using the arithmetic symbol, say yo uhave.

ndVector x(1);

them: ndVector y = x + x

is a valid operation in cpp, but if you try that in the python wrapper or c shaper, you will get an undefined function error. but you can do instead

y = x.Add(x)

and should works, because the operator+ is converted to member function Add.

all the functions are defined in the Newton.i swig script.

if you sync, it should just work, or be very closed to a wrapper with the full functionality or the engine.

perkinslr commented 2 years ago

As usual, you go beyond any reasonable expectation of assistance, and solve problems I didn't know about before they manifested. Thank you.

It is worth noting you can enable the y = x + x behavior in python by providing it as __add__ instead of (or as well as) .Add, but it is not uncommon to use foreign types through a purely functional interface, so explicitly calling .Add is not a problem.

I had not realized that setting NEWTON_BUILD_SHARED_LIBS=off would enable building the static libraries, but I suppose it's obvious in hindsight that you're going to build something, so static if not dynamic.

Anyway, I'll give it a test on windows again probably tomorrow. Hopefully the linker issues are resolved with the changes to the newton.i file.

It's worth noting the issues with ndFloat32 (and related) types that are typedeffed in sdk/dCore/ndTypes.h not auto-converting from the wrapped float type still persist. I copied from line 209 to line 226 of ndTypes.h into one of the headers in the swig directory and that resolved the issue. It chokes if I try to just add ndTypes.h to the list of included headers in the swig file, since I think it pulls some parts in twice.

JulioJerez commented 2 years ago

It is worth noting you can enable the y = x + x behavior in python by providing it as add instead of (or as well as) .Add, but it is not uncommon to use foreign types through a purely functional interface, so explicitly calling .Add is not a problem.

oh yes you are correct, somehow python automatically replaces operator with the name, it is c sharp that does not do that. I commented out that block in the Newton.i

It's worth noting the issues with ndFloat32 (and related) types that are typedeffed in sdk/dCore/ndTypes.h not auto-converting

ah yes that's another problem. yes, I was never able to add ndTypes.h to the swig script, because that file includes too many systems and standard header, and there, there are too many inline stuffs that swig has problem with.

instead, I did almost the same thing to did. but the minimal define are in header file ..\newtonPy\newtonConfig.h and ..\newtonSharp\newtonConfig.h

the problem was that the still had the legacy prefix d instead of nd. it is all fixed now. the glue code generate by swig, is in fact a lot cleaner since, for example, it removes functions to convert for ndInt32 to int and that for each function.

one last think is that I added ndVector.h instead of ndVectorSimd.h this has pros and cons, when using ndVectorSimd.h there is a Swig make a huge .cxx file when all the conversion from vector are inlined, and the makes the wrapper mush faster and much less memory activity. when using ndVector.h, it only knows that ndVector is a type, and make a call to make one object. this is more python style, but the wrapper is less efficient. you can check this by looking at some of the generated classes for example: the class ndBody.cs function GetAABB

using ndVectorSimd.h is generate as

  public void GetAABB(ndVector p0, ndVector p1) {
    newtonPINVOKE.ndBody_GetAABB(swigCPtr, ndVector.getCPtr(p0), ndVector.getCPtr(p1));
    if (newtonPINVOKE.SWIGPendingException.Pending) throw newtonPINVOKE.SWIGPendingException.Retrieve();
  }

but when using ndVector.h, it is generated as

  public void GetAABB(SWIGTYPE_p_ndVector p0, SWIGTYPE_p_ndVector p1) {
    newtonPINVOKE.ndBody_GetAABB(swigCPtr, SWIGTYPE_p_ndVector.getCPtr(p0), SWIGTYPE_p_ndVector.getCPtr(p1));
    if (newtonPINVOKE.SWIGPendingException.Pending) throw newtonPINVOKE.SWIGPendingException.Retrieve();
  }

it does not seem much, but SWIGTYPE_p_ndVector is a managed object, which goes through lot of memory and data conversion while ndVector is a native type. that was generted by the .cxx glue code.

however, those saving are so minor, that I think is better to just use the define type and not the inlined low level version and that will be more aligned with the target language.

anyway, try sync before you try again.

perkinslr commented 2 years ago

I'm a little confused about the practical difference between using ndVectorSimd.h and ndVector.h. ndVector.h generates a smaller newton_wrap.cxx, but at the expense of doing one extra boxing/unboxing step when using ndVectors. You say the performance impact is minimal (and I'm happy to accept that, given newton is otherwise so efficient it likely matters little), but I'm not sure what the advantage is (aside from a slightly smaller newton_wrap.cxx that is not really designed to be edited anyway). I suppose less code means maybe slightly easier tracebacks in the debugger when something goes wrong? If the performance proves to be important, it might be possible to switch between the headers for early development vs late development / production builds. Not sure about the added complexity of that vs the potential performance difference...

Anyway, this just compiles out-of-the-box on Linux now. There are still a couple rough edges though. CMake sets the final library name based on the project name, so the output is libnewtonSharp.so. swig embeds the DllImport lines to tell csharp where to find the library based on the swig module name. The swig module name can be set via -module {projectName} (so -module newtonSharp to match cmake), but otherwise defaults to the name of the swig interface file (so newton in this case). This means the resulting newton.dll is looking for a C library libnewton.so rather than libnewtonSharp.so. Given that we probably want the csharp binding to indicate in its name it's a csharp binding, it probably makes sense to either pass -module newtonSharp to swig, or to rename newtion.i to newtionSharp.i (which wouldn't match the python naming convention...), or to include a library mapping file to tell it that DllImport("newton" should load libnewtonSharp.so on linux and newtonSharp.dll on windows. I suppose the last option would be to, like the python version that copies to _newton.pyd, have a post-build step that copies libnewtonSharp.so to libnewton.so. Given that you can build it to only have one dynamic library, and that the shared newton library is libndNewton.so, there isn't even a name conflict on that.

Anyway, I'll try on windows here shortly and see if the symbol issue is resolved. Might try a clean dev environment if not, just to make sure the issue isn't local to the windows vm.

perkinslr commented 2 years ago

Can confirm it works on windows too.

JulioJerez commented 2 years ago

Ok, so we now know it work, so that the base line.

I do not know what difference it makes in python, using inline ndVectorSimd.h

in c chart its been few years since I use it. But in c sharp there are ways to get a native pointer to an object, so it seems that the swig compiler capitalize on that when it can resolve an inline function. So all those functions that pass vector around, are collapsed when they are inlined, you see that the .cs files take the genetic cshapt type. But when using the inline is does some funny casting in the .cxx function.

I do not worry about that, newton is a very lightweight library, and there is very little communication from the app to the sdk. So the time in .cxx is negligible. I rather like it better when not using tge inlines.

I belive that python does something simular. But I can't testify to that since I know very little about it.

On the library name. You can make a pull request, and them it can be added.

The one thing about cmake search for installed dependencies, is that in window not every one install app in the default folders, so many time many c make script fail to find the dependency, that why I used the environment variable.

If you make the pull request, I can check it out and if it works, them that's I merge it.

On Sun, Jul 10, 2022, 12:00 PM Logan Perkins @.***> wrote:

Can confirm it works on windows too.

— Reply to this email directly, view it on GitHub https://github.com/MADEAPPS/newton-dynamics/issues/285#issuecomment-1179780987, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6EPJDPSW2HTRL6AZCFMIDVTMMULANCNFSM53C27Z3A . You are receiving this because you commented.Message ID: @.***>