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

[Cleanup] Streamlined the physics simulation update routine #858

Merged
merged 13 commits into from May 9, 2016

Conversation

Projects
None yet
2 participants
@ulteq
Contributor

ulteq commented Apr 29, 2016

Read this first: #858 (comment)

  • Removed the BeamFactory worker thread
  • Merged threadentry() with UpdatePhysicsSimulation()
  • Moved UpdatePhysicsSimulation() from Beam to BeamFactory
  • Fixed crash when DisableThreadPool=Yes

Edit:

  • Moved all physics related updates into BeamFactory::calcPhysics
  • Chopped the Beam::frameStep into several smaller functions
  • Added new syncWithSimThread method, which is now called at the beginning of the RoRFrameListener::frameStarted() method
  • SimThread and MainThread no longer access the truck engine at the same time
  • updateFlexbodiesPrepare() and UpdateEvents() no longer run in parallel with the SimThread

@ulteq ulteq changed the title from [Codechange] Removed the beamfactory pthread worker to [Cleanup] Streamlined the physics simulation update routine Apr 30, 2016

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr May 2, 2016

Member

The PR comments sound pretty good.

I can't really evaluate the code, I lost track of the simulation logic. The 1st commit which replaces custom logic with std::*future looks great, other seem to mostly move code around. But as I said, I haven't worked with it in a while.

One thing - after you're done refactoring, please make a formatting sweep of Beam.cpp and BeamFactory.cpp (eventually elsewhere) - Transform all variables to underscored_lowercase, members to m_prefixed_lcase and functions to CamelCase(), add explict this-> to method calls. I'd like to avoid new/refactored code making formatting mess just because the code was messy before.

Member

only-a-ptr commented May 2, 2016

The PR comments sound pretty good.

I can't really evaluate the code, I lost track of the simulation logic. The 1st commit which replaces custom logic with std::*future looks great, other seem to mostly move code around. But as I said, I haven't worked with it in a while.

One thing - after you're done refactoring, please make a formatting sweep of Beam.cpp and BeamFactory.cpp (eventually elsewhere) - Transform all variables to underscored_lowercase, members to m_prefixed_lcase and functions to CamelCase(), add explict this-> to method calls. I'd like to avoid new/refactored code making formatting mess just because the code was messy before.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq May 2, 2016

Contributor

What about m_simulated_truck instead of simulatedTruck

I will do it like that, before we merge it. I've actually already renamed all of them, but it cluttered the diff and I didn't want to make the commits even harder to understand due to code reformatting.

Contributor

ulteq commented May 2, 2016

What about m_simulated_truck instead of simulatedTruck

I will do it like that, before we merge it. I've actually already renamed all of them, but it cluttered the diff and I didn't want to make the commits even harder to understand due to code reformatting.

@ulteq

This comment has been minimized.

Show comment
Hide comment
@ulteq

ulteq May 2, 2016

Contributor

other seem to mostly move code around.

Two main things changed:

  • The Beam::frameStep method is gone:
    That means we no longer have to call it on the ACTIVATED truck, to start the simulation of all other trucks
  • Our main thread code no longer runs in parallel with the sim thread
    Up until now this was the only code section which was executed while the sim thread was paused.

The sim thread now only runs while Ogre is doing its thing in the background, but not while we execute code in the main thread.

We wait for the sim thread to finish before we enter the main thread code section.
And we launch the sim thread after the main thread is done.

We should still move most of our code from the frameStarted event into the frameRenderingQueued event.

Contributor

ulteq commented May 2, 2016

other seem to mostly move code around.

Two main things changed:

  • The Beam::frameStep method is gone:
    That means we no longer have to call it on the ACTIVATED truck, to start the simulation of all other trucks
  • Our main thread code no longer runs in parallel with the sim thread
    Up until now this was the only code section which was executed while the sim thread was paused.

The sim thread now only runs while Ogre is doing its thing in the background, but not while we execute code in the main thread.

We wait for the sim thread to finish before we enter the main thread code section.
And we launch the sim thread after the main thread is done.

We should still move most of our code from the frameStarted event into the frameRenderingQueued event.

@ulteq ulteq removed the needs discussion label May 2, 2016

@only-a-ptr

This comment has been minimized.

Show comment
Hide comment
@only-a-ptr

only-a-ptr May 3, 2016

Member

It all makes sense now.
👍
Thanks

Member

only-a-ptr commented May 3, 2016

It all makes sense now.
👍
Thanks

ulteq added some commits Apr 29, 2016

[Codechange] Merged threadentry() with UpdatePhysicsSimulation()
Moved UpdatePhysicsSimulation from Beam to BeamFactory

Also fixes crash when 'DisableThreadPool=Yes'
[Codechange] Refactored Beam::frameStep
Moved all physics related updates into BeamFactory::calcPhysics

Partitioned Beam::frameStep inter several functions

Added new 'syncWithSimThread' method, which is now called at the beginning of the RoRFrameListener::frameStarted method

SimThread and MainThread no longer access the truck engine at the same time

updateFlexbodiesPrepare() and UpdateEvents() no longer run in parallel with the SimThread
[Cleanup] RoRFrameListener::frameStarted() cleanup
Removed duplicate code and misleading / obsolete comments
[Cleanup] Header cleanup
Renamed BeamFactory private methods

Removed dead code

Removed redundant includes
[Cleanup] Replaced some raw pointers with unique_ptr
Also got rid of the THREAD_SINGLE / THREAD_MULTI defines

@ulteq ulteq merged commit ad870e8 into RigsOfRods:master May 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ulteq ulteq deleted the ulteq:simthread-simplification branch May 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment