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

Streaming Interface #718

Merged
merged 9 commits into from
Jun 21, 2023
Merged

Streaming Interface #718

merged 9 commits into from
Jun 21, 2023

Conversation

ThomasNaderBMW
Copy link
Contributor

Reference to a related issue in the repository

#700

Add a description

A streaming interface is needed for e.g. powering a graphics engine with dynamic changing data in a higher frequency.

Some questions to ask:
What is this change?
New generic interface for powering other components with dynamic data.
Is this a bug fix or a feature? Does it break any existing functionality or force me to update to a new version?
It is a new feature.
How has it been tested?
It is tested with a similar interrface.

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

If you can’t check all of them, please explain why.
If all boxes are checked or commented and you have achieved at least one positive review, you can assign the label ReadyForCCBReview!

Sorry, something went wrong.

@ThomasNaderBMW ThomasNaderBMW self-assigned this Feb 23, 2023
@ThomasNaderBMW ThomasNaderBMW changed the title Feature/pp/visualization interface pr2 Streaming Interface Feb 23, 2023
@ThomasNaderBMW
Copy link
Contributor Author

// Approach 1: is_obsolete flags inside the messages (example see osi_object.proto)

  • need to change all the messages, clarify for what it is clearly used
     + however, if user does not care, it can be simply ignored
  • all information at one place

// Approach 2: Is shown below, id_lists (also for traffic lights etc.)
//repeated Identifier obsolete_moving_object_id = x;

  • this is the least impact. volume of data, and changes to message

// Approach 3: This message with an enum describing if content is to update or to delete.

  • two top level messages

@globberwops
Copy link
Contributor

globberwops commented Mar 23, 2023

Approach 1)

Pros:

  • perfectly backwards compatible
  • value can be completely ignored
  • no data overhead if not sent
  • best information locality
  • easy to use in impl

Cons:

  • Needs unambiguous documentation

Approach 2)

Pros:

  • perfectly backwards compatible
  • value can be completely ignored
  • no data overhead if not sent
  • least impact

Cons:

  • Needs external knowledge
  • bad information locality
  • comparison of multiple data sources

@ThomasNaderBMW
Copy link
Contributor Author

ThomasNaderBMW commented Apr 20, 2023

Regarding realising a streaming interface, these are the open questions/points:
- Why not the approach sending GroundTruth cyclic (with a delta encoding flag) + a obsolete_id-Message? Or an inherited mechanism?

  • HostVehicleData also part of the message? ==> So far yes
  • HostVehicleData: Can all the data be changed or is it allowed?
  • Are the so far included submessages in the PR correct? Also the approach to extend in the future
  • Environmental Subconditions are solved inside EnvironmentalConditions, not on a higher level
  • Descriptions need to be reworked
  • Documentation is not enough at the moment
  • The name of osi_streaming.proto is not 100% correct. How would you name it? One idea was more closer symetric delta..
  • What about controlling the external part?
  • What if a new agent pops up, how does he know the initial dataset?

@thomassedlmayer
Copy link
Contributor

thomassedlmayer commented Apr 26, 2023

Refering to earlier discussions, this interface was planned to improve performance for use cases like visualization or similar but did not intend to be a "real"/full delta encoding for OSI, right?
So I guess one reason why we opted for a separate message for this interface and did not aim to use the GroundTruth message with some indicator flag (+ obsolete_id list) may be that it would not be accidently misunderstood/misused as delta encoding. In contrast to delta encoding, this "partial update" approach would enable to send updates only for objects that are considered relevant for the receiver (from sender point of view) and omit non-relevant object updates, right? It would not necessarily result in a complete representation of the groundtruth at the receiver. You would not know what happened to objects that were not included in the update.

I'd like to clarify which of the following variants we are discussing:

  • Send relevant objects (even if unchanged), not send non-relevant objects, add objects to obsolete_id list that just became obsolete/non-relevant
  • Send only objects that changed, not send unchanged objects, add objects to obsolete_id list that are not part of GroundTruth anymore at all (= more like real delta encoding as alternative to current way of sending complete OSI GroundTruth)
  • Something else?

I thought, initially we would aim for the first variant or did I understand something wrong? I think it would make sense to have some precise definition of what we want to achieve with this interface and how it would work in practice because for me it is quite confusing and unclear what the general understanding of the group concerning this topic is.

Also, I think we agreed on not using delta updates on fields like position etc., right? So an update message still contains the complete position/orientation/etc. values just like in the "original" groundtruth. That's why I think, we should not include "delta" in the name. It could be easily misunderstood IMO.

@ThomasNaderBMW I did not yet receive an invitation for the separate meeting to discuss the streaming interface. What was the date/time again?

@ThomasNaderBMW
Copy link
Contributor Author

@thomassedlmayer : Thank you very much for your input.

I sent you the meeting, but you should already have gotten it. It is on 2nd May.

I agree on, that the whole object (moving object e.g.) has to be sent if a single value changes.

@ThomasNaderBMW
Copy link
Contributor Author

Special Meeting Performance & Packaging:
Regarding realising a streaming interface, these are the open questions/points:

  • Why not the approach sending GroundTruth cyclic (with a delta encoding flag) + a obsolete_id-Message? Or an inherited mechanism?
    ==>Not intended as delta encoding message (concerned with bandwith), it is more a streaming update message (concerned with latency, minimizing it).
    ==>Two Messages because semantics of classical GT-mechanism and Streaming-mechanism are too different to put it alltogether.
    HostVehicleData also part of the message?
    ==> So far yes. But needs to be repeated
    HostVehicleData: Can all the data be changed or is it allowed?
    ==> Yes can be changed and is allowed.
    Are the so far included submessages in the PR correct? Also the approach to extend in the future.
    Environmental Subconditions are solved inside EnvironmentalConditions, not on a higher level.
    Descriptions need to be reworked.
    Documentation is not enough at the moment.
    The name of osi_streaming.proto is not 100% correct. How would you name it? One idea was more closer symetric delta..
    ==> It is not delta encoding any more. So streaming is correct. But better suggestions are welcome!
    What about controlling the external part?
    What if a new agent pops up, how does he know the initial dataset?
    ==> If you want to use it for that you need to se the first frame.
    To the initial setup data/Resync:
    ==> Next Meeting Performance & Packaging

@ThomasNaderBMW
Copy link
Contributor Author

Performance & Packaging, 04.05.23:
New Open Points:

  • Sending new objects if they are calculated new or if a property has changed?
  • First time message is being sent, how to handle that?
  • Mention an e.g. use case in the description
  • Documentation figure needs to be done (M. Lemmer sends it to T. Nader)

No future evolution of OSI will therefore use field numbers equal to or greater than 10000.
Copy link
Contributor

Choose a reason for hiding this comment

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

What has changed here? Or is this highlighting some bug in the diff-function from GitHub?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably some CRLF/LF stuff? I'm working on some documentation additions on the StreamingUpdate message right now anyway. I'll try deleting and adding the last line. Maybe that does the trick.

@ThomasNaderBMW ThomasNaderBMW added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jun 15, 2023
@ThomasNaderBMW ThomasNaderBMW added this to the V3.6.0 milestone Jun 15, 2023
@ThomasNaderBMW
Copy link
Contributor Author

CCB Review, 19.06.23:

  • Can be merged.
  • DCO-Sign off will be done by Pierre Mai.

@ThomasNaderBMW ThomasNaderBMW added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Jun 19, 2023
@pmai pmai force-pushed the feature/pp/visualization_interface_PR2 branch from e6ce255 to 89b6e4d Compare June 21, 2023 10:26
ThomasNaderBMW and others added 7 commits June 21, 2023 13:38
Interface and message for streaming updates, as discussed and revised in
the workshops.

Signed-off-by: Thomas Nader <[email protected]>
Signed-off-by: Thomas Nader <[email protected]>
Signed-off-by: Pierre R. Mai <[email protected]>
Signed-off-by: Thomas Nader <[email protected]>
Signed-off-by: Pierre R. Mai <[email protected]>
Signed-off-by: Thomas Sedlmayer <[email protected]>
Add description to associate through Host Vehicle Data ID.

Signed-off-by: Thomas Nader <[email protected]>
- Align figure
- Add architecture information
- Add section to top-level interface

Signed-off-by: Thomas Sedlmayer <[email protected]>
thomassedlmayer and others added 2 commits June 21, 2023 13:38
@pmai pmai force-pushed the feature/pp/visualization_interface_PR2 branch from 89b6e4d to 9f9536f Compare June 21, 2023 11:38
@pmai pmai merged commit 9b47674 into master Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants