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

Wrong position for transformation #560

Closed
opera-aberglund opened this issue Dec 27, 2023 · 5 comments
Closed

Wrong position for transformation #560

opera-aberglund opened this issue Dec 27, 2023 · 5 comments
Assignees
Labels
Library For issues that effect the library and aren't specific to any particular application.
Milestone

Comments

@opera-aberglund
Copy link

opera-aberglund commented Dec 27, 2023

I'm testing this out now and I'm having a small issue with it. I removed the calls to UseLocation and UseAngle and replaced them with UseSweep. However, the resulting body doesn't have the same location, or "m_xf", as the one I serialized it from. It seems that the value of m_xf is set from the pos0 from the sweep, done from the GetTransformation here:

Transformation GetTransformation(const BodyConf& conf) noexcept
{
    return {conf.sweep.pos0.linear, UnitVec::Get(conf.sweep.pos0.angular)};
}

when the value of x_mf in the old body is the value of pos1 of the sweep. And I can't call UseLocation to fix this, because then I will loose the pos0 from the sweep.

I also had to set resetMassData to false, because otherwise I will also loose the pos0 from the sweep, but I guess I always have to avoid this function if I want an exact copy of a body?

Originally posted by @opera-aberglund in #549 (comment)

@louis-langholtz louis-langholtz self-assigned this Dec 27, 2023
@louis-langholtz louis-langholtz added the Library For issues that effect the library and aren't specific to any particular application. label Dec 27, 2023
@louis-langholtz
Copy link
Owner

louis-langholtz commented Jan 13, 2024

I've run into this problem before too. But is this more of a wrong position, or more of a it-doesn't-work-as-expected problem?? I'm wondering this myself.

Even when I recently added the ability to set the sweep, I ran into this. Also, I ran into the gotcha of having to set resetMassData to false.

@opera-aberglund
Copy link
Author

But is this more of a wrong position, or more of a it-doesn't-work-as-expected problem??

I'm not sure either. I really have to emphasise that when I filed these issues I was almost completely blind to how things logically should work; I was solely looking at how I can recreate an exact copy of a serialized world, and I just found I had to change this function getting pos0 to pos1 for the equality to be true.

@opera-aberglund
Copy link
Author

opera-aberglund commented Jan 18, 2024

I'm fairly certain that changing it to pos1 would be correct, because my game state gets desynchronized after serialization and deserialization if I just use the current pos0 in the function.

@louis-langholtz louis-langholtz added this to the 2.0 Release milestone Jan 19, 2024
@louis-langholtz
Copy link
Owner

louis-langholtz commented Jan 21, 2024

Some notes on this, at least for myself:

  1. In commit a709055, playrho/d2/BodyConf.hpp was changed to add support for configuring with Sweep information, instead of being limited just to location and angle (that form a Position that would set Sweep::pos0 and Sweep::pos1 to that value).
  2. This meant that then one variable (sweep), spoke for what used to be two separate variables (location, and angle), and could then also convey state independently for the other stage of the sweep (so sweep.pos0 and sweep.pos1 can now both be set to what that had been - and not just to the same Position).
  3. This also meant that Body::m_sweep could then be initialized by BodyConf::sweep, instead of having Body::m_sweep initialized to the more limited Sweep{Position{bd.location, bd.angle}}.
  4. The question at hand seems to be whether the appropriate semantics of the location, angle and the Transformation of the former BodyConf have been preserved for the new BodyConf while allowing the other Position of the Sweep to be set independently.
  5. The address of this question seems to be predicated on whether the changes to BodyConf match the behavior for Body with respect to functions like Body::GetTransformation, which returns Body::m_xf, which itself is set from GetTransform1 of the body's sweep, which in turn is the value of GetTransformation(sweep.pos1, sweep.localCenter).
  6. In other words, I'm seeing this as a relational consistency question.

So would it be more consistent for GetTransformation(const BodyConf& conf) to return GetTransform1(conf.sweep) (which is GetTransformation(conf.sweep.pos1, conf.sweep.localCenter)) than {conf.sweep.pos1.linear, UnitVec::Get(conf.sweep.pos1.angular)}? I believe so.

I'm gonna try this and see what unit testing reveals.

@louis-langholtz
Copy link
Owner

I've merged in changes now to the master branch to address this. Closing this assuming that it does. Please let me know though either way. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Library For issues that effect the library and aren't specific to any particular application.
Projects
None yet
Development

No branches or pull requests

2 participants