Feature/issue 544, 577, 578, 255 Weather Conditions #582

m-pauly · 2021-09-21T14:12:39Z

Signed-off-by: Markus Waldmann markus.waldmann@stud.hs-kempten.de & Markus Pauly markus.pauly@stud.hs-kempten.de

Reference to a related issue in the repository

Description

Introduction of the use of continuous values instead of enums for several weather-values.
More Information: See description in the several Issues.

… to OSI

…onditions to OSI and changed emuns to double values

caspar-ai

In general, I wonder if this PR is trying to tackle too much at once.

There are some uncontroversial additions within this PR, specifically the new data such as Wind, CloudState, and Sun. Perhaps it would be beneficial to split those into a separate PR which can get merged more easily into master and then create new PR which addresses the more controversial issue of how to extend what are currently single fields. In particular, that may require separate PRs to get merged into a 3.x and 4.x release stream if breaking changes are desirable in the long-term.

caspar-ai · 2021-09-30T14:32:30Z

osi_environment.proto

+    //
+    // \note Defines the cloud states according to OpenScenario 1.1
+    //
+    optional CloudState cloud_state = 10;


I would suggest CloudState should be a message which contains the current CloudState enum.

It isn't required now but is more robust to wanting to extend how cloud state is described in future. (if all the other fields in this structure had done that originally this PR probably wouldn't exist 🙂)

caspar-ai · 2021-09-30T14:33:51Z

osi_environment.proto

+    //
+    // \note Describes the wind speed and direction from OpenScenario 1.1
+    //
+    message Wind


This Wind message is never used anywhere, should there be an optional field referencing it above?

caspar-ai · 2021-09-30T14:51:49Z

I should also say, I'm happy to help with making those changes and PRs, but I didn't want to step on anyone's toes, if it would be helpful though, let me know.

jdsika · 2021-10-13T06:49:52Z

Is this planned to be done in 4.0? Maybe have a clean breaking change then extending strangely in order to align with OpenSCEANRIO 1.x and OpenLABEL? Is there already a decision here?

caspar-ai · 2021-10-13T08:18:48Z

@jdsika I would agree with that suggestion. There are some completely new parts that I think we could safely and sensibly include in 3.x, but I would agree that it would make more sense to make a breaking change to do this "properly" for the existing fields in 4.0.

jdsika · 2021-10-13T09:10:02Z

So I would leave out "XY Extended" and add the easy ones from OSC1.X. Is it "sun" e.g.? What is the status of the debate regarding "cloudState"?

Could you remove the ones we agree to propose for 3.X here, clean it up (there are some errors from deleting and adding the same messages -> rebase completely), create a draft with tag 4.0 from this one and add the others to a separate PR, please?

jdsika · 2021-10-13T11:54:44Z

@kmeids do you think my proposal is agreeable? I think we should also have a quick look in the compatibility with the tag list from OpenLABEL Public Review here as well. The goal should be a harmonized feature set which is not necessaryly loss free transformable (not needed IMO) but compatible in a sense like "sun from the front of the car" is able to be derived from "sun angles and ego vehicle position".

kmeids · 2021-10-13T19:41:32Z

Hello everyone,
@jdsika we have already discussed this topic in the SM group and did an alignment with open scenario and open ontology.
Eventually we did come up with a proposal that @jruebsam is still working on finalizing it.
@m-pauly, we have a SM meeting this Friday at 9:00 I'll forward you the invitation and hopefully @jruebsam will also be available with his proposal to discuss.

Changed the sun to a StationaryObject Update after meeting and new input given of the Sensor Group

ThomasNaderBMW · 2021-10-18T06:40:26Z

osi_environment.proto

+    message Precipitation
+    {
+        // The intensity of the precipitation (valid for all precipitation types). Unit [mm/h]. Range [0...inf[
+        // According to OpenScenario 1.1 and according to the context of ISO 2315


23150 ? Missing 0

ThomasNaderBMW · 2021-10-18T06:41:51Z

osi_environment.proto

        //
-        AMBIENT_ILLUMINATION_LEVEL4 = 5;
+        CLOUD_STATE_SLIGHTLYCLOUDY = 5;


I think should be CLOUD_STATE_SLIGHTLY_CLOUDY

ThomasNaderBMW · 2021-10-18T06:45:45Z

Waiting for 4.0 may make sense.
From content view I left some comments and also forwarded to colleagues that need such extensions.
Let's see what will come back, but I think it goes into the right direction!

stmswald · 2021-10-20T05:56:42Z

We will now create two new PR. One for the Issue #544 with wind, sun and cloud state with tag V3.4.0. And one PR for #577 and #578 with tag for V4.0.0 with no more enum values for the fog, precipitation and illumination.

jdsika · 2021-10-20T06:27:49Z

We will now create two new PR. One for the Issue #544 with wind, sun and cloud state with tag V3.4.0. And one PR for #577 and #578 with tag for V4.0.0 with no more enum values for the fog, precipitation and illumination.

I recommend to leave the ENUMS in the v4.0.0 and add a DEPRICATED comment above. I think it is better to evaluate with user behavior if the new description is accepted. The only important point is a note that both fields should be set and you need a transformation rule. But I think this is easily done.

jdsika · 2021-10-20T06:29:18Z

@kmeids feel free to hand out my email for someone to discuss this in depth as I am not available on Fridays (part time currently).

stmswald · 2021-10-20T07:00:43Z

We will now create two new PR. One for the Issue #544 with wind, sun and cloud state with tag V3.4.0. And one PR for #577 and #578 with tag for V4.0.0 with no more enum values for the fog, precipitation and illumination.

I recommend to leave the ENUMS in the v4.0.0 and add a DEPRICATED comment above. I think it is better to evaluate with user behavior if the new description is accepted. The only important point is a note that both fields should be set and you need a transformation rule. But I think this is easily done.

Sure, for the first draft for V4.0.0. we can add the idea of @jruebsam from issue #255:

precipitation.label -> Enum
precipitation.state -> Double

and keep the idea of enums and double values?

stmswald · 2021-10-20T18:19:34Z

@thomassedlmayer this PR can be closed and not merged. The new PR for V3.4.0 is now PR #593. The PR for V4.0.0 will follow in within the next week

m-pauly · 2021-10-20T19:04:09Z

@stmswald PR should be closed now.

Markus Waldmann and others added 5 commits September 6, 2021 15:16

feature/Issue #544 Extend: Added OpenSCENARIO Enviromental conditions…

9aa82a1

… to OSI

feature/Issue #577 and #578 Extend: Added OpenSCENARIO Enviromental c…

03852e1

…onditions to OSI and changed emuns to double values

feature/Issue #577 and #578 Extend: Corrected to OSI convention -signoff

872bd77

Change of Cloudstate-Description #577 #578 –signoff

9cbee3f

Merge branch 'OpenSimulationInterface:master' into master

d36966e

0815-code linked an issue Sep 21, 2021 that may be closed by this pull request

Environmental Conditions #255

Closed

thomassedlmayer changed the base branch from feature/sm/environment-conditions-update to master September 21, 2021 14:52

Todo's from Meeting included

24930ea

caspar-ai reviewed Sep 30, 2021

View reviewed changes

jdsika added this to the V4.0.0 milestone Oct 13, 2021

jdsika added the Concept An issue that is being detailed out through expert discussion and offline work label Oct 13, 2021

Added FogMist and changed Precipitation to repeated

269a0b2

Added reference models to Precipitation and FogMist

445a810

Changed the sun to a StationaryObject Update after meeting and new input given of the Sensor Group

ThomasNaderBMW reviewed Oct 18, 2021

View reviewed changes

Merge branch 'master' into master

325bbb7

stmswald mentioned this pull request Oct 20, 2021

Environmental conditions, added Sun, Wind, CloudState #593

Merged

m-pauly closed this Oct 20, 2021

PhRosenberger self-assigned this Mar 1, 2023

PhRosenberger changed the title ~~Feature/issue 544, 577, 578 Weather Conditions~~ Feature/issue 544, 577, 578, 255 Weather Conditions Mar 3, 2023

PhRosenberger mentioned this pull request Apr 24, 2023

Environmental conditions DRAFT for V4.0.0. #601

Closed

9 tasks

Feature/issue 544, 577, 578, 255 Weather Conditions #582

Feature/issue 544, 577, 578, 255 Weather Conditions #582

Uh oh!

Conversation

m-pauly commented Sep 21, 2021 • edited by PhRosenberger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference to a related issue in the repository

Description

Uh oh!

caspar-ai left a comment

Choose a reason for hiding this comment

Uh oh!

caspar-ai Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

caspar-ai Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

caspar-ai commented Sep 30, 2021

Uh oh!

jdsika commented Oct 13, 2021

Uh oh!

caspar-ai commented Oct 13, 2021

Uh oh!

jdsika commented Oct 13, 2021

Uh oh!

jdsika commented Oct 13, 2021

Uh oh!

kmeids commented Oct 13, 2021

Uh oh!

ThomasNaderBMW Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

ThomasNaderBMW Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

ThomasNaderBMW commented Oct 18, 2021

Uh oh!

stmswald commented Oct 20, 2021

Uh oh!

jdsika commented Oct 20, 2021

Uh oh!

jdsika commented Oct 20, 2021

Uh oh!

stmswald commented Oct 20, 2021

Uh oh!

stmswald commented Oct 20, 2021

Uh oh!

m-pauly commented Oct 20, 2021

Uh oh!

Uh oh!

m-pauly commented Sep 21, 2021 •

edited by PhRosenberger

Loading