Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Oct 22, 2018

This PR extends static objects and moving objects by a relative path to 3D models. This information can be used to model sensors that require precise geometrical information of their surrounding.

@ghost ghost requested a review from pmai October 22, 2018 12:23
@ghost ghost mentioned this pull request Oct 22, 2018
@ghost ghost changed the title Adding relative model path to objects Adding relative model path to objects fixes #285 Oct 22, 2018
@ghost ghost added FeatureRequest Proposals which enhance the interface or add additional features. Suggestions I just want to drop by and leave this suggestion to think about. labels Oct 22, 2018
@ghost ghost added this to the v3.1.0 milestone Oct 22, 2018
@ghost ghost self-requested a review October 22, 2018 12:35
@pmai
Copy link
Contributor

pmai commented Oct 29, 2018

As a general point I think we should very carefully consider, whether talking about file paths in OSI is a good idea, since that presupposes that the is access to some common file system between different simulation models, which is not generally true (i.e. OSI-based simulations can run on distributed systems with no single system image).

The most portable solution would be to transport all needed data during setup messages, and reference that data in the per time-step messages, however this might too complex a solution for 3.1.0 for now.

A sort of middle ground would be to add fields like

// Opaque idenitifer of an associated 3D model of the stationary object. 
// 
// \note It is implementation-specific how model_ids are resolved to 3d models.
optional string model_id = 4; 

which would make the implementation-specific part of this information explicit, while still having a single place where to put this kind of information (instead of relying on implementation-specific field additions). And in future revisions it would be possible to add the setup machinery needed to transmit the 3d models during startup.

@ghost
Copy link
Author

ghost commented Oct 30, 2018

Hi @pmai,
I could well live without the relative file path.
The latter can be set in the user-specific implementation.
Nonetheless - wouldn't "model_name" be more precise than "model_id"?!

@pmai
Copy link
Contributor

pmai commented Oct 30, 2018

@LudwigFriedmannBMW it could also be model_key, the idea is that the string value is just an abstract key to locate a model, whether it is a relative path, a model name or any other abstract id is left open. I.e. in certain implementations it would be a relative path, because the models share a common file system, in others it would be a database primary key used for lookup, in others it might be a key value store, etc.

A name is both more specific and less specific, in my opinion, since it is not clear that it can/should be used to uniquely identify the model being referenced. So maybe model_reference would also be a good name.

Ludwig Friedmann added 2 commits November 5, 2018 14:15
Changing model_path to model_reference and altering documentation accordingly
@ghost ghost modified the milestones: v3.1.0, v4.0.0 Nov 20, 2018
@ghost
Copy link
Author

ghost commented Dec 4, 2018

After considering potential inconsistencies and/or ambiguousities in the implementation of the concept I postpone its implementation to OSI 4.0.0. A revision of the concept of object types could be helpful here.

@ghost ghost modified the milestones: v4.0.0, v3.1.0 Dec 21, 2018
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost merged commit ed6e42c into OpenSimulationInterface:master Dec 21, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FeatureRequest Proposals which enhance the interface or add additional features. Suggestions I just want to drop by and leave this suggestion to think about.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants