Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update support for buggy android NDK #1419

Merged
merged 2 commits into from
Nov 8, 2017

Conversation

kwasimensah
Copy link
Contributor

Expand logic for when __thread is supported to work around android/ndk#8

Expand logic for when __thread is supported to work around android/ndk#8
@erwincoumans
Copy link
Member

OK, let me have a look and merge it today, Thanks!

@erwincoumans
Copy link
Member

Mac OSX platform is affected by this change. Could you rewrite the patch, using

#if defined(__ANDROID__) 
 //some new logic, 
#else 
unmodified existing logic 
#endif

then there is less chance it breaks other platforms.

Don't take away ability to do profiling on MacOS
@kwasimensah
Copy link
Contributor Author

Ok, pushed new version. Will wait for tests to pass

@kwasimensah
Copy link
Contributor Author

Tests have passed. I think this can be integrated now.

@erwincoumans
Copy link
Member

Since the logic is more complex than #if defined(ANDROID) #else #endif I'll have to manually go through the code and test if this doesn't break on any platform.

@erwincoumans
Copy link
Member

Thanks for the pull request!

@erwincoumans erwincoumans merged commit ee2f568 into bulletphysics:master Nov 8, 2017
@erwincoumans
Copy link
Member

OMG, this patch seems to be broken indeed: 8aefa7e#commitcomment-25547649

So it gets reverted and I'll have to manually change the logic to not break things....

@kwasimensah
Copy link
Contributor Author

Pull request is at #1431

@AndresTraks
Copy link

The new patch fixes it.

BT_THREADSAFE is defined when BULLET2_USE_THREAD_LOCKS is set in CMake, so you'd have to build two configurations if you want to test this. It makes sense for it to be a preprocessor macro, because otherwise, with the way multi-threading is done now, an "isThreadSafe" variable would have to be checked a lot during simulation.

As an exception, I think btQuickProf should be made thread-safe by default or at least redesigned to not use static variables, because it crashes in the case where you want to simulate completely separate worlds in separate threads, which has nothing to do with Bullet's multi-threading functionality: http://bulletphysics.org/Bullet/phpBB3/viewtopic.php?t=5588

@erwincoumans
Copy link
Member

erwincoumans commented Nov 16, 2017

Good point. btQuickProf/BT_PROFILE is used for benchmarking, and we mainly use the newer and better b3ProfileZone/B3_PROFILE instead which is more flexible (in combination with Bullet/examples/Util/ChromeTraceUtil.cpp). At some stage, I'll have to retire LinearMath and move to Bullet3Common, have to figure out when/how to avoid breaking lots of code bases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants