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.

Sign up for GitHub

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

Jump to bottom

Environmental conditions, added Sun, Wind, CloudState #593

Merged
merged 14 commits into from
May 2, 2022
Merged

Environmental conditions, added Sun, Wind, CloudState #593

merged 14 commits into from
May 2, 2022

Conversation

stmswald
Copy link
Contributor

@stmswald stmswald commented Oct 20, 2021 •
edited
Loading

With this PR we try to fulfill the needs of harmonization of the Issue #544, #577, #578 as well as Issue #255 and the PR #582
The discussion has been over a year now and we try to merge with small steps.

So far we only added the Sun, CloudState as well as the Wind as messages.
This should help merging this PR in Version V3.5.0. more easily.

There is another PR #601 in order to get the Enum types and continues values for e.g. Fog, Precipitation, Ambient Illumination to V4.0.0. #577 #578 #255

According to #544 the following data structures were created within this PR:

  • Sun:
    • azimuth [rad]
    • elevation [rad]
    • intensity [lx]
  • Wind:
    • direction [rad]
    • speed [m/s]
  • CloudStates (enum):
    • Level 1 - 8 to tell how many clouds cover the blue sky

most of these requirements are form ASAM openSCENARIO

@stmswald
Copy link
Contributor Author

@jdsika please add this to Milestone V3.4.0.

@stmswald stmswald mentioned this pull request Oct 20, 2021
Closed
caspar-ai
caspar-ai reviewed Oct 20, 2021
View reviewed changes
Copy link
Contributor

@caspar-ai caspar-ai left a comment

Choose a reason for hiding this comment

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

One main comment about putting the CloudState enum inside a parent message to make future extensions or changes easier.

All the other comments you can use or ignore as you would like!

osi_environment.proto Outdated Show resolved Hide resolved
osi_environment.proto Outdated Show resolved Hide resolved
osi_environment.proto Outdated Show resolved Hide resolved
osi_environment.proto Outdated Show resolved Hide resolved
osi_environment.proto Show resolved Hide resolved
@thomassedlmayer thomassedlmayer added this to the V3.4.0 milestone Oct 21, 2021
@thomassedlmayer thomassedlmayer added the Harmonisation The Group in the ASAM development project working on harmonisation with other standards. label Oct 21, 2021
@HendrikAmelunxen HendrikAmelunxen added this to To do in Harmonisation via automation Oct 26, 2021
@HendrikAmelunxen HendrikAmelunxen moved this from To do to In progress in Harmonisation Oct 26, 2021
HendrikAmelunxen
HendrikAmelunxen approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@HendrikAmelunxen HendrikAmelunxen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@HendrikAmelunxen HendrikAmelunxen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Oct 27, 2021
@HendrikAmelunxen HendrikAmelunxen moved this from In progress to ReadyForCCB in Harmonisation Oct 27, 2021
@kmeids kmeids modified the milestones: V3.4.0, V4.0.0 Oct 27, 2021
@stefancyliax stefancyliax changed the title Environmental conditions, added Sun, Wind, CloudState for V3.4.0. Environmental conditions, added Sun, Wind, CloudState Nov 10, 2021
@stefancyliax
Copy link
Contributor

Edited the title for clarity. The release of 3.4.0 is closed now. This will be considered in an upcoming 3.5.0 release.

@stefancyliax stefancyliax modified the milestones: V4.0.0, V3.5.0 Nov 10, 2021
@0815-code 0815-code linked an issue Nov 12, 2021 that may be closed by this pull request
Environmental Conditions #255
Closed
@stmswald stmswald mentioned this pull request Nov 23, 2021
Draft
9 tasks
kmeids
kmeids reviewed Nov 24, 2021
View reviewed changes
osi_environment.proto Outdated Show resolved Hide resolved
kmeids
kmeids reviewed Nov 24, 2021
View reviewed changes
osi_environment.proto Outdated Show resolved Hide resolved
kmeids
kmeids reviewed Nov 24, 2021
View reviewed changes
osi_environment.proto Outdated Show resolved Hide resolved
kmeids
kmeids reviewed Nov 24, 2021
View reviewed changes
osi_environment.proto Outdated Show resolved Hide resolved
kmeids
kmeids reviewed Nov 24, 2021
View reviewed changes
osi_environment.proto Outdated Show resolved Hide resolved
@kmeids kmeids removed the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Nov 24, 2021
@HendrikAmelunxen HendrikAmelunxen moved this from ReadyForCCB to In progress in Harmonisation Nov 30, 2021
@stmswald stmswald force-pushed the feature/ha/environmental_conditions_minor branch from e3b1e6c to 3666c20 Compare December 21, 2021 13:21
@thempen thempen moved this from In progress to ReadyForCCB in Harmonisation Dec 21, 2021
@kmeids
Copy link
Contributor

kmeids commented Apr 13, 2022

Output CCB 13.04.2022:

  1. Review the following: https://github.com/OpenSimulationInterface/open-simulation-interface/pull/593/files#r849287153
  2. To reiterate on optional int64 unix_timestamp = 8;
  3. DCO is also missing.

@kmeids kmeids removed the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Apr 13, 2022
@pmai
Copy link
Contributor

pmai commented Apr 13, 2022

DCO Signoffs are only technically broken, will be fixed prior to merging by @pmai.

@HendrikAmelunxen HendrikAmelunxen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Apr 29, 2022
@pmai pmai 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 May 2, 2022
@pmai
Copy link
Contributor

pmai commented May 2, 2022

CCB 2022-05-02: Can be merged, @pmai will do the merge and correct the DCOs.

stmswald and others added 14 commits May 2, 2022 10:58
@stmswald @pmai
Added wind, cloud state and sun to environment
1a9f422 
Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>
@stmswald @pmai
Rework wind, cloud, and sun definitions in environment
da5f946 
- Move cloud state enums into CloudState message

  The enum has been changed to CloudStateLevel and moved into a more
  open meaning message of CloudState. This can easily be expanded in the
  future with more detailed information.

- Generalize definition text of the wind

  The message within the description was too specific, if in the future
  updates will probably have more than only the wind speed and direction
  in it.

- Definition text changed Sun and Wind

  The definition within the Sun and Wind messages were indicating that
  data must originate from an OpenSCENARIO file.

- Definition text rules and unit Sun changed

  The definition within the Sun message was not divided into units,
  rules and description

- Fixes to field numbering, nesting, etc. to align with interface rules

Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>
@stmswald @pmai
Rework cloud, sun, and wind in environment
01154ed 
- Remove LEVEL_NO_SKY in CloudStateLevel

  There will be no need for now for this state, if no sky is needed, use
  LEVEL_OTHER.

- Describe rules within description

  With rules there is no greater than pi possible. Therefore, moved the
  rules to the description.

- Change wind description

  Changed the range to -pi to +pi like in common.proto for Oreintation3d
  messages. Added a note for minding the conversation from openSCENARIO.

Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>
@stmswald @kmeids @pmai
Further rework of environment additions sun, cloud
c75adc8 
- Move TimeOfDay message back to avoid confusion

- Change the message name Clouds to CloudLayer

- Change the SUN angular range

- Wind direction is now origin not target

Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>

Co-authored-by: Kmeid <kmeid.saad@ansys.com>
@stmswald @pmai
Changed naming in CloudLayer, added Sky Obscured
cba8940 
Renamed ENUMS to LEVEL0-8, added LEVEL_SKY_OBSCURED for situations where the sky is not perceivable, Visual Reference in the comments added

Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>
@stmswald @pmai
Changed descriptions in CloudLayer
777e4d1 
Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>
@pmai @kmeids
Apply suggestions from code review for environment additions
267c327 
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>

Co-authored-by: Kmeid <kmeid.saad@ansys.com>
@stmswald @pmai
Changed descriptions for Sun and Wind
4035f8f 
Harmonisation group output: Rework of range and counting methods for azimuth, elevation and wind direction.

Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>
@stmswald @pmai
Changed cloud, wind descriptions to fulfill comments
3d90725 
Changed Definition -> Description several times
Changed description texts
Changed Ranges of wind direction
Changed ENUM cloud layer
ENUM CloudLayerLevel -> FractionalCloudCover
Added new reference
changed naming and description text

Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>

Changed ENUM FractionalCloudCover

Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>
@stmswald @pmai
Changed description of cloud cover
76759d7 
changed the references

Signed-off-by: Markus Waldmann <Markus.Waldmann@stud.hs-kempten.de>
@thomassedlmayer @pmai
Fix minor doc formatting issues
5108f23 
Signed-off-by: thomassedlmayer <thomas.sedlmayer@tum.de>
@thomassedlmayer @pmai
Change sun elevation description
920897d 
Signed-off-by: thomassedlmayer <tsedlmayer@pmsfit.de>
@thomassedlmayer @pmai
Add description to unix timestamp
8ecfa77 
Signed-off-by: thomassedlmayer <tsedlmayer@pmsfit.de>
@thomassedlmayer @pmai
Specify observation point for sun
8c2b1db 
Signed-off-by: thomassedlmayer <tsedlmayer@pmsfit.de>
@pmai pmai force-pushed the feature/ha/environmental_conditions_minor branch from 2e51ee3 to 8c2b1db Compare May 2, 2022 09:14
@pmai pmai dismissed thomassedlmayer’s stale review May 2, 2022 09:22

Has been resolved.

@pmai pmai merged commit b289970 into OpenSimulationInterface:master May 2, 2022
@HendrikAmelunxen HendrikAmelunxen moved this from ReadyForCCB to Merged/Done in Harmonisation May 6, 2022
@HendrikAmelunxen HendrikAmelunxen mentioned this pull request May 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmai pmai pmai left review comments

@kmeids kmeids kmeids left review comments

@HendrikAmelunxen HendrikAmelunxen HendrikAmelunxen approved these changes

@caspar-ai caspar-ai caspar-ai left review comments

@ThomasNaderBMW ThomasNaderBMW ThomasNaderBMW left review comments

@thomassedlmayer thomassedlmayer thomassedlmayer left review comments

Assignees

@pmai pmai

@HendrikAmelunxen HendrikAmelunxen

@ThomasNaderBMW ThomasNaderBMW

Labels
Documentation Everything which impacts the quality of the documentation and guidelines. Harmonisation The Group in the ASAM development project working on harmonisation with other standards. ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
Harmonisation
Merged/Done
Milestone
V3.5.0
Development

Successfully merging this pull request may close these issues.

Environmental Conditions: Missing documentation Environmental Conditions
8 participants
@stmswald @stefancyliax @kmeids @pmai @HendrikAmelunxen @caspar-ai @ThomasNaderBMW @thomassedlmayer