o3de / o3de-extras

Other
61 stars 61 forks source link

In many places in the code, deltaTime, that is not a real deltaTime is used to compute speeds, feedback, and other #709

Closed michalpelka closed 2 months ago

michalpelka commented 3 months ago

Describe the bug This is not a bug, but rather a technical debt. Fortunately, for us it is small. ROS 2 Gem simulation can use different time sources and work non-realtime (slower/faster than realtime). In many places this wrong pattern is used:

    void JointsTrajectoryComponent::OnTick(float deltaTime, [[maybe_unused]] AZ::ScriptTimePoint time)
    {
        uint64_t deltaTimeNs = deltaTime * 1'000'000'000;
        FollowTrajectory(deltaTimeNs);
    }

This is clearly wrong - deltaTime is a time difference in wall time, not a simulation clock. We should do smth like that:

    void JointsTrajectoryComponent::OnTick([[maybe_unused]] float deltaTime, [[maybe_unused]] AZ::ScriptTimePoint time)
    {
        const auto timestamp = ROS2Interface::Get()->GetROSTimestamp();
        const deltaTime = ROS2::Utils::GetDiffInSec(timestamp, m_lastTimestamp);
        uint64_t deltaTimeNs = deltaTime * 1'000'000'000;
        FollowTrajectory(deltaTimeNs);
        m_lastTime = timestamp;
    }

Assets required No asset - it is an error found in the codebase during review.

Steps to reproduce There is no reproduction.

Expected behavior The whole ROS 2 Gem should respect the used simulation clock.

Actual behavior Part of the code-base uses a wall-time clock.

Found in Branch All branches.

jhanca-robotecai commented 2 months ago

The issue is solved, I will close it for now. Please reopen the issue if needed.