Feature: Added Dead Reckoning (DR) Module#83
Conversation
Implemented standard DIS Dead Reckoning algorithms in opendis/DeadReckoning.py . Added unit tests in tests/test_dead_reckoning.py . Added usage example in examples/dis_dead_reckoning.py . Algorithms support Static, FPW, RPW, RVW, FVW, FPB, RPB, RVB, FVB modes.
Implemented standard DIS Dead Reckoning algorithms in opendis/DeadReckoning.py . Added unit tests in tests/test_dead_reckoning.py . Added usage example in examples/dis_dead_reckoning.py . Algorithms support Static, FPW, RPW, RVW, FVW, FPB, RPB, RVB, FVB modes.
|
@JayshKhan thank-you! @ngjunsiang would you be interested in helping to review? |
|
@leif81 @JayshKhan this is a challenging PR to review for a few reasons: Review challengesUnclear use case for open-dis-pythonAlthough I'm a contributor to this codebase, I am not using it in any practical application yet (I'm primarily doing so while doing some research for a military simulation project I have not started on yet). From the reported issues and PRs so far I am thus not sure how this package is intended to be used: as a plugin to a working simulator for emitting PDUs, as a deserializer for viewing logged PDUs, as a teaching aid, ... Uncertain API designI think this matters because the optimal API very much depends on the use case and I am hesitant to make a judgement on how to lock in the API until the use cases become clearer. For example, if open-dis-python is primarily used for introspection of PDUs, the implementation of DRM might not matter so much since the simulation will likely have its own implementation. But if used as a simulation plugin, then the implementation begins to matter. So far I am primarily following @leif81's guideline on following the IEEE1278.1 specification faithfully when it comes to class and attribute naming. But the Dead Reckoning Model (DRM) describes algorithms for which the input and output type/format become critical to performance. Potential performance optimizationsWe want to avoid as much overhead as possible for algorithms which will be invoked often, and which are likely to be further optimized as the codebase develops further. Ideally we want to get as much of the API "correct" from the start as possible, to minimize disruption to package users (and the need to bump major version numbers). While Python is not primarily known for speed, I don't have a good feel for where this boundary is for the users of open-dis-python and the DRM algorithms, which further adds to the hesitation to make major decisions so early in the package's implementation (Proposed) Guidelines for new featuresDespite the uncertainty we are operating in, I think this is a useful place to check in with other contributors on some principles. Maintain optionality until use cases are clearerWe should avoid locking in API designs until we know how a class or function will be used. Prefer private interfaces and avoid committing to public interfaces until a use case is demonstrated. Avoid huge classes/functions with opaque functionality, preferring small classes/functions that are easy to compose. Minimise implementation until discussion progressesWhile discussion is underway for API design and use cases, we should keep implementation light (avoid unnecessary assertions and validation), focusing on the core functionality and keeping optionality open for other enhancements. That said, I think this PR mostly fits these requirements and would be a good fit for the package in its current state with some changes. ReviewDRM Purpose and useIEEE1278.1 states the use of the Dead Reckoning Model as follows:
It is thus expected that helper functions will be needed that can take a threshold, an entity, and a dead-reckoned model, and figure out if the DRM threshold has been exceeded. Generic Input, Specific OutputSince it is unclear how entity state (position, velocity, acceleration) will ultimately be represented in use cases, the principle of Generic Input, Specific Output suggests flexibility in the input types (i.e.
De-abstract implementationThere are 9 DRM algorithms in total. Each algorithm takes in a single entity's state, and returns a new state: position, velocity, acceleration, orientation, angular velocity. This makes it seem ripe for a Strategy design pattern. I am thus hesitant to OK the current pattern of grouping all 9 algorithms under a namespace class ( May I suggest that for this PR we keep the decision lock-in small and just implement them as separate functions in The import them becomes
NitpicksIn the algorithms the vectors are generally unpacked into variables, e.g. psi, theta, phi = orientation
p, q, r = angular_velocityBut in return np.array([
[0, -omega[2], omega[1]],
[omega[2], 0, -omega[0]],
[-omega[1], omega[0], 0]
])I would recommend following the same pattern to reduce cognitive overload: wx, wy, wz = omega
return np.array([
[0, -wz, wy],
[wz, 0, -wx],
[-wy, wx, 0]
])A detailed review of the algorithmic implementation would take more time and I probably won't be able to get to it until next week. But I also think that is not too critical at this juncture, and should not block this PR from being merged once the above are addressed. Any bugs can still be reported and fixed. |
Yes, it's meant to be added as a library in a python app to send and receive PDU's. |
I agree. If there are bugs or questions about the DR algorithm implementations here, I would suggest comparing it with the implementation in the open-dis-java project: And it's unit tests: |
|
@ngjunsiang thank-you for providing the review. One of the reasons I ask is I'm more comfortable with Java, much less with Python. So having yourself comment on and recommend best practices for Python is very helpful. Thank-you for that |
There was a problem hiding this comment.
@JayshKhan are you able to ammend this PR with these 2 suggestions provided by @ngjunsiang in #83 (comment)
- Change annotations for DRM functions (e.g. drm_rpw()) to accept Sequence types for input.
It may be helpful to define a type alias (e.g. Vector = typing.Sequence) to make this easier; if the Vector type definition needs to change in future then only a one-line change is needed. - in the algorithms unpack the vectors into variables to reduce cognitive overload:
|
Hey @leif81, @ngjunsiang — thanks for the great feedback! I’ll go ahead and update the PR with those suggestions. I’ll modify the DRM function annotations to use I’ll get the changes pushed soon |
- Update type hints to accept generic Sequence inputs (lists/tuples) - Improve variable unpacking in matrix helper functions for readability
|
@JayshKhan thank-you that was quick! @ngjunsiang are you good with the changes? |
|
looks good to me 👍 |
Feature: Add Dead Reckoning (DR) Module
This PR implements the standard IEEE 1278.1 Dead Reckoning (DR) algorithms for
open-dis-python.The Problem — Why This Is Needed
The DIS protocol requires Dead Reckoning so simulators can move entities smoothly between network updates (typically every 5 seconds for non-maneuvering entities).
Without Dead Reckoning:
The Solution — What This PR Adds
A new module:
This module provides a clean, static API implementing all standard IEEE 1278.1 DR algorithms:
Key Attributes
Code Comparison
Before
Users manually computed physics:
After
Standard DR function with correct behavior:
Verification & Tests
Included in this PR:
Unit tests:
tests/test_dead_reckoning.pyExample script:
examples/dis_dead_reckoning.pyAll tests pass.
Dependencies
Impact
This PR upgrades
open-dis-pythonfrom a packet-level parser into a viable simulation engine component.It enables: