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

Inconsistencies in Graph Sdu's transform list requirements #59

Open
ckwichael opened this issue Jan 17, 2023 · 3 comments
Open

Inconsistencies in Graph Sdu's transform list requirements #59

ckwichael opened this issue Jan 17, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@ckwichael
Copy link

ckwichael commented Jan 17, 2023

I apologize since this issue is somewhat pedantic, but I found some ambiguities in the TransformList property of the GraphSdu.

  • In Figure 12 of Section 7.2.4, in the GraphSdu the type of the transformList is FrameTransformIndexPair, should this be an array of FrameTransformIndexPair? I believe that many of the graphs will generally have at least n-1 pairs where n is the number of nodes in the graph which would indicate that we need an array of pairs.
  • Requirement 8 specifies that the graph is a directed acyclic graph where each node is extrinsic or reachable from an extrinsic node so we could potentially have a graph of n extrinsic nodes and no connections, is this correct or is the intent for the graph to be a directed acyclic connected graph? Note 1 in section 4.2.16 seems to suggest that having no connections is acceptable, but I just want to confirm that for my own knowledge.
  • Figure 12 of section 7.2.4 describes the FrameTransformIndexPair datatype as having two members: outerFrameIndex and innerFrameIndex. However in the json schema in section 9.2.5 the transformList is describes as having a type FrameTransformPair rather than FrameTransformIndexPair and this type seems to have an array of link objects that simply contain an array. I can intuit that the first value is likely the outerFrameIndex and the second value is the innerFrameIndex but that is not explicitly stated in any requirement that I can find. Since this is a directed graph it seems that it is very important to be explicit in which index is which. My personal preference is the data type specified in the UML, that is, the FrameTransformIndexPair which has two explicit members. It removes any ambiguity and, in my opinion, makes it easier to work with.
  • Requirement and Conformance Test 21 seems to ambiguously list the type of transformList as FrameListTransformPair or FrameTransformIndexPair. It is unclear whether the intent was that the FrameListTransformPair was simply a list of FrameTransformIndexPair, a typo, or a remnant from and older version of the spec.

I think that I'm able to intuit the spirit of the SDU, but someone else may interpret one of these requirements slightly differently or make a different assumption about the way the data should look which could damage interoperability between Geopose users. I think it would be beneficial to ensure that all of the requirements and datatypes are explicit and consistent.

@3DXScape
Copy link
Collaborator

It is really helpful to have your review!

  • Yes, it looks to me like it is an error. It should be an array of FrameTransformIndexPair s.
  • Yes, it is possible to have a disconnected graph, even one where all of the connected subgraphs have just one node. While not useful if you want to transform between disconnected components, it can represent reality. For example, building a world model in real time from sensors on a mobile platform, there may be disconnected chunks that mostly become connected as more data is acquired.
  • Yes, we should make the referents of the indices explicit in the UML.
  • The FrameListTransformPair vs FrameTransformIndexPair is both a typo and based on evolution of the spec. Pretty much all these and probably similar other occurrences should use the FrameTransform... terminology. We will add a scan of the whole document to unify this.

Thanks again for taking the time to interpret thie document and point out errors and inconsistencies.

@3DXScape 3DXScape added the bug Something isn't working label Jan 20, 2023
@3DXScape
Copy link
Collaborator

  • Make it an array of FrameTransformIndexPair s.

  • Make the referents of the indices explicit in the UML.

  • Scan of the whole document to unify usage of FrameListTransformPair vs FrameTransformIndexPair - similar other occurrences should use the FrameTransform... terminology.

@cperey
Copy link
Collaborator

cperey commented Jan 20, 2023

ACTION: Document this and any other bugs found in an FAQ or the developers' guide (before summer release of corrigendum 1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants