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

Change 'static' to 'not changing' in the specification of GroundTruthInit #108

Conversation

ClemensLinnhoff
Copy link
Contributor

@ClemensLinnhoff ClemensLinnhoff commented Mar 22, 2024

Reference to a related issue in the repository

#107

Add a description

Extend GroundTruthInit specification to also cover constant properties of other GroundTruth data, such as the model_reference in MovingObject.

Mention a member

@pmai @LudwigFriedmann

Check the checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation for osi-sensor-model-packaging.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests / Github Actions pass locally with my changes.

…etween static objects and non-changing, i.e. constant, values.

Signed-off-by: ClemensLinnhoff <[email protected]>
@@ -5,17 +5,17 @@ endif::[]
= Ground truth initialization parameters

All models can optionally consume `osi3::GroundTruth` via an initialization parameter called `OSMPGroundTruthInit`.
Its purpose is to provide the model with a view of the static environment, for example the map, in OSI format.
Its purpose is to provide the model with information that is constant throughout the simulation, for example the map in OSI format or the model_reference of a MovingObject.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's discuss the wording for static/constant/moving/dynamic for changes in OSI3.8, as it also touches topics as in OpenSimulationInterface/open-simulation-interface#768.

How about: Its purpose is to provide the model with information that does not change throughout the simulation, for example the map in OSI format or the model_reference of a MovingObject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generally changed 'static'/'constant' to not 'changing'

Signed-off-by: ClemensLinnhoff <[email protected]>
@PhRosenberger PhRosenberger added documentation Everything which impacts the quality of the documentation and guidelines. ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Mar 25, 2024
@ClemensLinnhoff ClemensLinnhoff changed the title Change 'static' to 'constant' in the specification of GroundTruthInit Change 'static' to 'not changing' in the specification of GroundTruthInit Mar 25, 2024
Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

The suggested changes try to still identify the data as static, as this is the language we use in other parts of OSI (e.g. omit_static_information in SensorViewConfiguration), and avoids mentioning MovingObject, as we have no real way of providing only truly static parts of MovingObject in GroundTruthInit,.

doc/spec/ground_truth_init_parameters.adoc Outdated Show resolved Hide resolved
doc/spec/ground_truth_init_parameters.adoc Outdated Show resolved Hide resolved
doc/spec/ground_truth_init_parameters.adoc Outdated Show resolved Hide resolved
@ClemensLinnhoff ClemensLinnhoff requested a review from pmai March 27, 2024 15:11
@LudwigFriedmann
Copy link

Is my understanding correct that the GroundTruthInit defines the content that is likely to remain static over the course of the simulation? A GroundTruth received later can still overwrite this content, right?

Is it sufficient to give examples, or must it be clearly stated that, for example, model_references can be specified for StationaryObjects, MovingIObjects and the environment model specified by the global model_reference?

@tbleher
Copy link

tbleher commented Mar 27, 2024

I don't think it will work to just define some properties (e.g. the model_reference) as constant (at least this is my current understanding). Let me explain:
The original idea was that GroundTruthInit will only contain objects that do not change during the simulation run. This meant that a model (like OSIPedestrian) could simply merge GroundTruthInit with the latest SensorView. In practice, this causes a few problems: it means that no traffic lights can be transmitted, since their status changes at runtime. But OSIPedestrian would like to have the position of all traffic lights at init time (to create a static map which includes all places where a traffic light controls access to the road). So in practice we use the GroundTruthInit in a way that it contains all traffic lights, but the status is updated later in the SensorView. So in that sense I think one can add objects to the GroundTruthInit that change their state later.

However, this breaks down for objects that are added or removed at runtime (most often vehicles, since these are spawned and removed at runtime, but other object types are also affected). Let me give an example:

  • GroundTruthInit should contain all signs, so that the 3D models can be loaded before simulation start. omit_static_information is true.
  • Later, a SensorView is received, without a specific sign. Now there are two possibilities:
    1. The sign is not sent since omit_static_information is set. This is an important performance improvement, and is the current interpretation.
    2. The sign was removed as part of road works.

Previously, the second scenario was solved by NOT adding it to GroundTruthInit. Any object that may be removed during simulation (or is not available at start) MUST NOT be in the GroundTruthInit!
As far as I understand, there is no way to distinguish these two cases, if all objects are part of the GroundTruthInit (so the 3D models can be loaded).

I see basically two way to solve this (though there may be more):

  • Add a separate list of 3D models to be loaded to the GroundTruthInit (at which point it probably makes sense to give the GroundTruthInit its own type, since now it is really an initialization message, and no longer a generic Ground Truth).
  • Add a list of objects to the SensorView that are in the GroundTruthInit but not currently valid. Note that this would have to be ALL objects not currently valid, since SensorView are not incremental.

@ClemensLinnhoff
Copy link
Contributor Author

However, this breaks down for objects that are added or removed at runtime (most often vehicles, since these are spawned and removed at runtime, but other object types are also affected).

Would it be an option to specify, that all objects with parameters defined in the GroundTruthInit have to be present in the simulation at all times?

And I would say, parameters that are not defined in the GroundTruthInit can be changed during runtime, e.g. the traffic light position is constant, but not the state or the moving object model_reference is constant but not its position.

@tbleher
Copy link

tbleher commented Mar 27, 2024

Would it be an option to specify, that all objects with parameters defined in the GroundTruthInit have to be present in the simulation at all times?

I think it would work in the sense that we would get a consistent model. I think it isn't what @LudwigFriedmann wants, though - my understanding is that he wants to know ALL 3D models at initialization, so they can be loaded before the simulation starts. I think this is not possible without field changes, at least not without gross hacks (like moving all "invalid" objects to positions far outside the sensor field, but still always transmitting them).

And I would say, parameters that are not defined in the GroundTruthInit can be changed during runtime, e.g. the traffic light position is constant, but not the state or the moving object model_reference is constant but not its position.

But how can you detect which parameters are defined in the GroundTruthInit? You can know whether an object is defined or not, since its id must be != 0. But with Proto3, any field equal to 0 is not serialized. So I think you cannot know e.g. if the position was not set, or if the object is at 0/0/0.

@tbleher
Copy link

tbleher commented Mar 27, 2024

Correction:

You can know whether an object is defined or not, since its id must be != 0.

I think this is wrong from my last comment. I think the Id is allowed to be 0, but it works since the field which e.g. contains the moving objects is repeated, and Protobuf correctly deserializes it.

But I think the point still stands: for an indidual object, you cannot know which fields are set, unless you define poison fields (like UNKNOWN in OSI), give these the value 0 and make it clear that they may never be set in the ground truth. Then you know that any field which has a poison value hasn't been set.

@ClemensLinnhoff
Copy link
Contributor Author

But I think the point still stands: for an indidual object, you cannot know which fields are set, unless you define poison fields (like UNKNOWN in OSI), give these the value 0 and make it clear that they may never be set in the ground truth. Then you know that any field which has a poison value hasn't been set.

Is that really the case? So the has_position() function would return false if it is set to 0,0,0? I guess we need to test that.

@ClemensLinnhoff
Copy link
Contributor Author

I think it isn't what @LudwigFriedmann wants, though - my understanding is that he wants to know ALL 3D models at initialization

But in that case I would argue that it might not be the best idea to spawn and remove objects during the simulation runtime anyways.

@tbleher
Copy link

tbleher commented Mar 27, 2024

But in that case I would argue that it might not be the best idea to spawn and remove objects during the simulation runtime anyways.

I think the use case (wanting to load all 3D models at initialization, but still dynamically spawning and despawning) is a valid one, just not one that the current structure can support.

To give two examples:

  • an endurance run on a HIL setup
  • experiencing a longer drive (> 30min) in a driving simulator

In both scenarios, it is important that everything runs in realtime (which means models HAVE to be pre-loaded) but one also wants to spawn and de-spawn vehicles dynamically.

@tbleher
Copy link

tbleher commented Mar 28, 2024

But I think the point still stands: for an indidual object, you cannot know which fields are set, unless you define poison fields (like UNKNOWN in OSI), give these the value 0 and make it clear that they may never be set in the ground truth. Then you know that any field which has a poison value hasn't been set.

Is that really the case? So the has_position() function would return false if it is set to 0,0,0? I guess we need to test that.

I thought this was the case, but did further tests. It looks like my previous comment was wrong, for which I apologize!

Here is the test program I used:

#include "osi_object.pb.h"
#include <iostream>
#include <string>

static void
serializeDeserializeAndPrint(const osi3::MovingObject& obj, const char* state)
{
    std::string buffer;
    obj.SerializeToString(&buffer);
    osi3::MovingObject deserialized;
    deserialized.ParseFromString(buffer);
    std::cout << "Size of " << state << ": " << buffer.size();
    if( deserialized.has_base() ) {
        std::cout << "; has base";
        if( deserialized.base().has_position() ) {
            std::cout << "; has position";
        }
    }
    std::cout << '\n';
}

int main(int argc, const char* argv[])
{
    osi3::MovingObject mo;
    serializeDeserializeAndPrint(mo, "empty");
    mo.mutable_base();
    serializeDeserializeAndPrint(mo, "with base");
    mo.mutable_base()->mutable_position();
    serializeDeserializeAndPrint(mo, "with position");
    mo.mutable_base()->mutable_position()->set_x(1);
    serializeDeserializeAndPrint(mo, "with position (x=1)");
    mo.mutable_base()->mutable_position()->set_x(0);
    serializeDeserializeAndPrint(mo, "with position (x=0)");
    mo.set_model_reference("");
    serializeDeserializeAndPrint(mo, "with empty model reference");
    mo.set_model_reference("a");
    serializeDeserializeAndPrint(mo, "with non-empty model reference");
    mo.clear_model_reference();
    serializeDeserializeAndPrint(mo, "with cleared model reference");
    return 0;
}

For proto2 files (as are checked in into OSI), the result looks like this (on Ubuntu 22.04 with libprotobuf 3.12.4 as shipped with Ubuntu):

Size of empty: 0
Size of with base: 2; has base
Size of with position: 4; has base; has position
Size of with position (x=1): 13; has base; has position
Size of with position (x=0): 13; has base; has position
Size of with empty model reference: 15; has base; has position
Size of with non-empty model reference: 16; has base; has position
Size of with cleared model reference: 13; has base; has position

After running convert_to_proto3.sh, the results look like this:

Size of empty: 0
Size of with base: 2; has base
Size of with position: 4; has base; has position
Size of with position (x=1): 13; has base; has position
Size of with position (x=0): 4; has base; has position
Size of with empty model reference: 4; has base; has position
Size of with non-empty model reference: 7; has base; has position
Size of with cleared model reference: 4; has base; has position

From this I conclude that even with proto3 it is possible to find whether a struct has been set or not (which is nice :)). But it is not possible to know whether an individual field has been set or not, since 0 or the empty string are not serialized.

@tbleher
Copy link

tbleher commented Mar 28, 2024

The fact that it is possible to determine whether the position is set could be a way forward for this patch: we could define that objects (static objects, moving objects, traffic lights and traffic signs) which are in GroundTruthInit but whose position is not set are not actually spawned statically and are only visible if they are found in the SensorView. But their model_reference could be set in GroundTruthInit, enabling the loading of the 3D models before the start of the simulation.

@pmai
Copy link
Contributor

pmai commented Mar 28, 2024

As I said at the time, currently there is too little definition to reliably include only part of an object in the GroundTruthInit, as we have no mechanism to specify which parts of the object should be recognized: Since the mixture of structured vs. scalar fields is more or less random, there would have to be very specific rules that spell out which fields should always be ignored, which fields should be considered, whether object existence is included or not, etc.

In order to do any of this we would likely also have to have more agreement of what fields are generally considered static and what fields are not (e.g. currently nothing even states that the model reference has to stay static, and there are likely even some use cases where changes make sense). Since this work has not been finished for the current release, I see no way to make any of this explicit in the current release.

That being said, anyone is free to have their own special handling, as most of what GroundTruthInit is already relies on some beyond-standard agreements between simulator and model, as e.g. the content of GroundTruthInit already is up to those kinds of agreements.

Personally, for the specific case of the model_references I would not go with GroundTruthInit in any case: As the meaning of the references is already implementation-defined, and not the actual model but rather just a key is transferred, there must already be an out-of-bounds communication channel to communicate the models in the first place (i.e. the file system or some database connection, etc.). With that in place I would adjust this channel to also carry the information of which models are relevant (e.g. by only having relevant models distributed, or by having a simple file enumerating them also be distributed).

@ClemensLinnhoff
Copy link
Contributor Author

Personally, for the specific case of the model_references I would not go with GroundTruthInit in any case

Yes, there has to be an off-standard agreement between ground truth supplier and sensor model. But it still has to be initialized in the co-simulation. And I do think GroundTruthInit can be used for this.

@pmai
Copy link
Contributor

pmai commented Apr 2, 2024

Personally, for the specific case of the model_references I would not go with GroundTruthInit in any case

Yes, there has to be an off-standard agreement between ground truth supplier and sensor model. But it still has to be initialized in the co-simulation. And I do think GroundTruthInit can be used for this.

As discussed above, anything can of course be done on a 1:1 agreement base, but that is completely unrelated to OSI, then. Since we have no mechanisms to clearly specify this at the moment, I personally would not go down this route. And if you just want to know which models to load on simulation startup, given that you need e.g. a file system interface to load them from, I would just use this channel to communicate, which seems easier to do and has less problems.

That being said it is usually a Bad Idea(tm) to expect real-time performance at simulation time 0 in any case, as usually a number of things are being calculated/done for the first time, and hence caches/TLBs are cold, memory might be unallocated, etc., so that largish random spikes in execution time are not unexpected. If you need real-time performance, then one usually uses pre-run time to make sure that everything has settled in prior to real-time guarantees being required.

@tbleher
Copy link

tbleher commented Apr 3, 2024

I can understand that OSI might not be ready yet for this task. But completely giving up on this seems wrong - we certainly need this standardized at some point. Since if we we need again to have separate agreements with each vendor - what is the point of having a standard at all?

And I disagree that this is just a "run the simulation for a few seconds" case: if I run a driving simulation for half an hour, then I certainly do not want to have spikes 25 minutes in since the renderer needs to load additional 3D models it hasn't seen so far. I think there really should be a mechanism to say "these are the 3D models I will need". And for us, the filesystem will at best be a hacky workaround - then you have to modify the filesystem for each new scenario, for something that should be transmitted as part of the initialization.

@pmai
Copy link
Contributor

pmai commented Apr 3, 2024

I can understand that OSI might not be ready yet for this task. But completely giving up on this seems wrong - we certainly need this standardized at some point. Since if we we need again to have separate agreements with each vendor - what is the point of having a standard at all?

I do not disagree. At some point in time this is likely an issue for OSI; however what needs to happen first (or at the same time) is a standardization of the actual 3D model access: What's the point of getting a list of possible model_references when the model and simulator still need to make agreements on what these even mean, how to access them and what format of 3D model they represent?

And that is my point: As long as this is all non-standardized, I see little point in codifying an ad-hoc mis-use of GroundTruthInit to supply the list of potential model references. For most current use-cases, there is going to be a file access channel to access the models referenced, and that channel can be used to communicate the potential set just as well; if not, then a custom field is just as portable as any ad-hoc codification, so that is an option as well.

Once we make head-way on the 3D model access front, it is of course a good idea to communicate potential models as well via some OSI channel. Whether that is going to be part of an enhanced GroundTruthInit, or some other mechanism is yet to be seen, given that I see little work being done in this direction yet.

And I disagree that this is just a "run the simulation for a few seconds" case:

I never said it was just a run the simulation for a few seconds case, I pointed out that even if you get the list of models in advance you are likely going to need to run the simulation beforehand if you really need real-time performance (whether you should run in real-time is another question). And hence for some use-cases that pre-run is entirely sufficient, as you are running with a constant set of moving objects for the duration of the simulation (because even adding MOs on the fly without the 3D part can cause hick-ups, so you try to avoid it).

For other use cases, like the long running real-time simulation with changing sets of models you allude to, you will need more than this. But here you probably also do not want to have a fixed set communicated at init time, but rather might want to have a dynamic pre-announcement of new MOs/models during run-time, as the set of possible models might be too large to keep in memory depending on resolution, and/or the scenario is actually dynamic, hence the set of models is hard to predict beforehand.

And for us, the filesystem will at best be a hacky workaround - then you have to modify the filesystem for each new scenario, for something that should be transmitted as part of the initialization.

See above: Whether the filesystem is a good way to communicate this or not heavily depends on the use-cases: For highly parallel multi-job simulations this can actually be the best way to transport the information; in other cases a field in OSI is a good solution, and whether this really is an at initialization or a pre-announcement thing is again heavily dependent on the use case.

So I would welcome it if people who are interested in actually transmitting 3D model information via OSI would get together and work on this for the next OSI project duration.

Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

CCB 2024-04-04: Merge as-is.

@pmai pmai added ReadyToMerge and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Apr 4, 2024
@pmai pmai merged commit 596af22 into master Apr 4, 2024
5 checks passed
@pmai pmai deleted the 107-adjust-groundtruthinit-to-initialize-model_references-of-moving-objects branch April 4, 2024 09:01
@pmai pmai added this to the V1.5.0 milestone Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Everything which impacts the quality of the documentation and guidelines. ReadyToMerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust GroundTruthInit to initialize model_references of moving objects
5 participants