Skip to content

Modifies EntityStateStore application to include support for dead-reckoning#94

Open
met4000 wants to merge 8 commits into
openlvc:masterfrom
met4000:nrt-entity-dead-reckoning
Open

Modifies EntityStateStore application to include support for dead-reckoning#94
met4000 wants to merge 8 commits into
openlvc:masterfrom
met4000:nrt-entity-dead-reckoning

Conversation

@met4000

@met4000 met4000 commented Oct 22, 2025

Copy link
Copy Markdown
Contributor

Modifies EntityStateStore to use the new DrEntityStatePdus, allowing dead-reckoning to be performed on stored entities.
Provides support for all 9 dead-reckoning algorithms in the DIS v7 specification.

Resolves: #93

@met4000 met4000 self-assigned this Oct 22, 2025
@met4000 met4000 force-pushed the nrt-entity-dead-reckoning branch 3 times, most recently from da6bd02 to 6c74aad Compare October 23, 2025 10:40
@met4000 met4000 marked this pull request as ready for review October 23, 2025 10:42
@met4000 met4000 changed the title DeadReckoningStore application for entity dead-reckoning support DeadReckoningModelStore application for entity dead-reckoning support Oct 23, 2025
@michaelrfraser

Copy link
Copy Markdown
Contributor

Thanks Nathan, this all looks good. No philosophical issues, just a few structural questions and code formatting changes requested to keep things consistent.

Major themes are:

  • I'd suggest merging the Dead Reckoning Store with the Entity store so that all entity related things are in one spot
  • Use Logger vararg overloads rather than formatting strings inline
  • Ensure javadoc is filled out for all public methods and classes
  • A couple of notes on code style that probably aren't apparent unless explained :)

Comment thread codebase/src/java/disco/org/openlvc/disco/utils/Quaternion.java Outdated
Comment thread codebase/src/java/disco/org/openlvc/disco/utils/Quaternion.java Outdated
Comment thread codebase/src/java/disco/org/openlvc/disco/utils/Quaternion.java Outdated
@met4000 met4000 force-pushed the nrt-entity-dead-reckoning branch from 6c74aad to edfd53c Compare October 24, 2025 04:34
Comment thread codebase/src/java/disco/org/openlvc/disco/application/deadreckoning/DrmState.java Outdated

@michaelrfraser michaelrfraser left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Nathan, just had a re-read of your latest changes and have a few more comments (again mostly documentation and logging related).

Might be also worth tagging in @timpokorny as this is getting closer to completion.

@met4000 met4000 force-pushed the nrt-entity-dead-reckoning branch from 5cbff51 to e8dca74 Compare October 28, 2025 04:01
@met4000

met4000 commented Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

Looks like the order I pushed changes in while rebasing this time did manage to mess up the comparison that github links to. This should be the recent changes: https://github.com/openlvc/disco/compare/edfd53ce7a23ee45adb25f40804b13131e06eab5..e8dca7462fa88abb47bfe80ca31b2e41790fee21

@timpokorny

Copy link
Copy Markdown
Member

Just starting to look at this, but unfortunately I have some other afternoon commitments. I'll be back later so I will attempt to re-pick up, but I am formulating some thoughts around the store. I need to actually be able to articulate them better, so let me browse more first, but broadly speaking I'm not sure the functionality belogs at the store level. I also share the questions about the marker interface on EntityStatePDU, which feels like its bringing semantics from the "application" layer into the core area which is only meant to be about seralizliation/deserialization.

Let me look at it more to articulate what I think is needed and I'll come back.

Small Naming Stuff
Again, haven't go through properly yet, but like all terrible reviews I got some small/easy fix stuff first. On that front the one thing that jumped out at me was just the verbosity in some of the namings for the PDU stuff. I think we can trim down the naming in a number of places. Stuff like:
- DeadReckoningParameter.getEntityAngularVelocity() can just become DeadReckoningParameter.getAngularVelocity()
- DeadReckoningParameter.setDeadReckoningAlgorithm() can just become DeadReckoningParameter.setAlgorithm()
- and so on.

I'd take a look at what's there and trim down some of the names.

I'll come back with more articulated guidance on the application level stuff shortly.

@timpokorny

Copy link
Copy Markdown
Member

Right, first things first, as a first step for getting DR into the framework this is excellent. All the hard work done of sorting out the enhancements to pull DR data out, and then do something intelligent with it, including the utility math to do the work is done and in place. To be honest, that's 94.2% of the world.

My only major comment would be on infrastructure at the moment. There are a couple of changes I'd suggest that can reduce the infrastructure "touch" and keep it much lighter, while also keeping things feeling more consistent with the existing stuff.

Notes

  • I concur with your feeling on the IDeadReckoningEnabled marker interface. The pdu package to me is really only meant to deal with serializing and deserializing PDUs. More "extended" stuff that helps a developer start to manage their data like this belongs in the application package and kind of layers on top of the core stuff. So pushing an interface downward from application to pdu feels a bit like mixing concerns.

  • I also don't think we necessarily need a separate store for dead-reckoned things. All the existing stores have a bit of consistency to them in terms of API and approach at the moment, and the DeadReckoningModelStore doesn't fit with those. It's also the only one that wraps around another store, which feels like it's making another layer, but only half of one.

There are a few other small things like that where I think structurally at the moment it's like a hexagonal shape for an octagonal hole - it's close, but not quite smooth.

Suggestions
Dead-recknoning as defined in DIS is implicitly "utility", in that its pretty much stateless. That might not be entirely true if you want to do some cache for efficiency, but for the most part it is true.

To ligten some of the infrastructure, I'd suggest the following (noting there may be reasons why we can't, so this is just opening suggestions):

  • In the application package, I'd create a DrEntityStatePdu that extends EntityStatePdu

    • This class will add overrides that return values factoring in dead reckoning
    • It should go in the application package, because it's no longer just about ground-truth data, serialization/deserialization, which is what the app package "should" be limited to in theory
  • I'd put into that class getDrWorldLocation(long), getDrOrientation(long) (and any other methods where, depending on DR algorithm, they get calculated) where the param is the targetTime

    • Those methods will look up the algorithm from the PDU, calculate a value and return it
    • Might be inefficient if there is a call to getDrWorldLocation immediately after a call to getDrOrientation, so could calculate all value and then cache request for the last 3 target times to help subsequent lookups
    • If doing that, I'd make DrmState an inner class of DrEntityStatePdu
  • There are times where some sim apps will want to ignore the algorithm that the PDU says it should use, and use one of their choosing

    • To support this, I'd add overloads of getDrWorldLocation(DeadReckoningAlgorithm,long) (and same for all the others) to let users just be explicit about the values they want returned
  • I would update the EntityStateStore to return DrEntityStatePdu instances instead of EntityStatePdu instances. People can ignore the dead-reckoning methods if they want.

  • I'd remove package application.deadreckoning

    • No longer need the IDeadReckoningEnabled interface
    • DrmState could become inner class of DrEntityStatePdu, or if it is needed to help DR other things, I'd just put in a application.utils package or some such
    • All the model classes effectively do is provide a single method. These are entirely stateless (which is excellent) so they can just be put into the pdu.field.DeadReckoningAlgorithm enum. That will make it a bit big, so if it's too ugly, again just pull the methods out into a util class.

Benefits

  • No need to register models or keep track of them by entity or any tracking infrastructure
  • Don't need a separate store, and the consistency between the stores remains
  • Access patterns all remain the same
  • Don't need the IDeadReckoningEnabled marker interface
  • Condenses all the model classes down into the enum, or at worst a couple of stateless util classes

Thoughts solicited!

@met4000

met4000 commented Oct 30, 2025

Copy link
Copy Markdown
Contributor Author

@timpokorny I like those suggestion on how to structure it. A few discussion points:

  • There are times where some sim apps will want to ignore the algorithm that the PDU says it should use, and use one of their choosing

    • To support this, I'd add overloads of getDrWorldLocation(DeadReckoningAlgorithm,long) (and same for all the others) to let users just be explicit about the values they want returned

DrEntityStatePdu sounds like it's meant to be 'an entity but incorporating dead-reckoning information'. Keeping a DrEntityStatePdu associated with only one algorithm makes it more straightforward to do caching of results. EntityStateStore will likely still be storing EntityStatePdus internally anyways (edit: to get the largest benefits of caching, it should probably store DrEntityStatePdus, but the rest of the question is still valid), so instead of overloads of DrEntityStatePdu::getDrWorldLocation etc. with an alternate dead-reckoning algorithm, would it make sense to have DrEntityStatePdu EntityStateStore::getEntityState(EntityId) and DrEntityStatePdu EntityStateStore::getEntityStateWithAlgorithm(EntityId, DeadReckoningAlgorithm), adding an accessor to EntityStateStore that allows for an algorithm override?

Another point, although I'm not sure if we do anything about this; one advantage of having the dead-reckoning store separate from EntityStateStore is we can have different heartbeat timeouts for each (or even no timeouts for dead-reckoned entities). A dead-reckoned entity will usually have a slower heartbeat than a 'general' entity, so a timeout best suited for one is likely not suited for the other. It seems like we don't currently have mixed heartbeat rate support anyways, so maybe this is an issue we deal with at a time where that gets added, and we assume that the current rather long heartbeat timeout should work for both.

@timpokorny

timpokorny commented Oct 30, 2025 via email

Copy link
Copy Markdown
Member

@met4000 met4000 force-pushed the nrt-entity-dead-reckoning branch from e8dca74 to b1bda26 Compare October 30, 2025 08:07
@met4000

met4000 commented Oct 30, 2025

Copy link
Copy Markdown
Contributor Author

I wasn't expecting more discussion to have happened so soon and did end up switching over to having it based on DrEntityStatePdu - still would be good to discuss these points tomorrow (e.g. how to 'override' the DR algorithm) and see what you think of this current implementation.

@met4000 met4000 force-pushed the nrt-entity-dead-reckoning branch from b1bda26 to fcea34d Compare October 31, 2025 07:42
@met4000 met4000 marked this pull request as draft October 31, 2025 10:20
@met4000

met4000 commented Oct 31, 2025

Copy link
Copy Markdown
Contributor Author

WIP/TODO from discussion:

  • allow for overriding the algorithm when making dr request to DrEntityStatePdu (including caching alongside timestamp)
  • error if the timestamp requested for dead-reckoning is in the past
  • flag/config to disable dead-reckoning (make all algorithms behave like Static)
  • body coordinate DR alg implementations
    • basic test for circular path (should return to original position)
  • coordinate system handling
    • DeadReckoningAlgorithm support for testing what coordinate system an algorithm uses - new enum?
    • methods for switching DrmState between coordinate systems (static on DrmState?)
    • switch between coordinate systems if a DrEntityStatePdu algorithm override is of a different coordinate system

@met4000

met4000 commented Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

Not sure where application flags such as the one to disable dead-reckoning should be added - DiscoConfiguration as a Flag (description says Flags are 'PDU parsing/processing flags')? DiscoConfiguration as a system property? Some new config handler on DisApplication? Currently going with system property loaded into DiscoConfiguration.

@met4000 met4000 force-pushed the nrt-entity-dead-reckoning branch 2 times, most recently from 24cfa1f to 86e027e Compare November 6, 2025 07:21
@met4000 met4000 marked this pull request as ready for review November 6, 2025 07:22
@met4000

met4000 commented Nov 6, 2025

Copy link
Copy Markdown
Contributor Author

... and we're back open for business. All DR Algorithms are now implemented, with rudimentary tests for RVW and RVB (the ones with 'all' features enabled, using world and body coordinates respectively). To perform dead-reckoning, instead of using DeadReckoningModelStore, EntityStateStore now gives DrEntityStatePdus. And dead reckoning can be disabled via disco.app.deadreckoning.enabled, causing the calculations to be skipped and things to behave as if static.

Might still need to shuffle some things around (open to suggestions on changing interfaces etc.), but this should at least be most of the functionality.

@met4000 met4000 requested a review from timpokorny November 6, 2025 07:48
@met4000

met4000 commented Nov 6, 2025

Copy link
Copy Markdown
Contributor Author

I'll still wait for Michael to give this a thorough look before merging, but @timpokorny interested to see if this new setup is in line with what you were thinking.

Adds a number of methods to the Quaternion and Vec3 utility classes.
Notably adds support for Quaternion -> EulerAngles conversion, rotating
Vec3s by Quaternions, and introduces tests for Quaternion functionality.
DisApplication services and helpers used to be loaded before the config
for the DisApplication was loaded. If they relied on config options then
they would only be initialised with the defaults, even if the config
used to initialise the DisApplication had been modified. This commit
reorders the DisApplication constructor to load the config first before
starting the services and helpers.
Modifies EntityStateStore to provide dead-reckoning models for entities,
providing support for querying the entity's estimated position.

See: openlvc#93
@met4000 met4000 force-pushed the nrt-entity-dead-reckoning branch from eb6ac75 to 020e102 Compare November 7, 2025 04:18
@met4000 met4000 changed the title DeadReckoningModelStore application for entity dead-reckoning support Modifies EntityStateStore application to include support for dead-reckoning Nov 7, 2025

@timpokorny timpokorny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor pieces to look at, but overall this is looking really good.

Comment thread codebase/src/java/disco/org/openlvc/disco/application/EntityStateStore.java Outdated
Comment thread codebase/src/java/disco/org/openlvc/disco/application/pdu/DrEntityStatePdu.java Outdated
Comment thread codebase/src/java/disco/org/openlvc/disco/application/pdu/DrEntityStatePdu.java Outdated
Comment thread codebase/src/java/disco/org/openlvc/disco/application/pdu/DrEntityStatePdu.java Outdated
Comment thread codebase/src/java/disco/org/openlvc/disco/application/pdu/DrEntityStatePdu.java Outdated
Comment thread codebase/src/java/disco/org/openlvc/disco/application/pdu/DrEntityStatePdu.java Outdated
Comment thread codebase/src/java/disco/org/openlvc/disco/application/utils/DrmState.java Outdated
@met4000

met4000 commented Nov 25, 2025

Copy link
Copy Markdown
Contributor Author

I've updated my IDE to catch more formatting issues. These new changes should (mostly) fix the line width and some other formatting issues I noticed, and also implement those other suggestions.

@met4000 met4000 force-pushed the nrt-entity-dead-reckoning branch from da8eeee to 2d54df6 Compare February 9, 2026 08:54
@met4000

met4000 commented Feb 9, 2026

Copy link
Copy Markdown
Contributor Author

Some minor modifications; the recent commits have been renamed to make their naming consistent and clearer how they will be rebased in the previous commits. The latest commit has new formatting fixes and code tweaks to make Sonarqube happy(er).

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for DIS entity dead-reckoning

3 participants