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

Supporting several additional OpenScenario 1.0 tags #987

Closed

Conversation

XiaoFei9704
Copy link
Contributor

@XiaoFei9704 XiaoFei9704 commented May 23, 2023

Checklist:

  • [ √ ] Your branch is up-to-date with the master branch and tested with latest changes
  • [ √ ] Extended the README / documentation, if necessary
  • [ √ ] Code compiles correctly and runs
  • [ √ ] Code is formatted and checked with Utilities/code_check_and_formatting.sh
  • [ √ ] Changelog is updated

Description

We've added some new tags that weren't supported before, as shown in Changelog.md and openscenario_support.md.

Where has this been tested?

  • Platform(s): ubuntu 20.04
  • Python version(s): python 3.8.10
  • Unreal Engine version(s): UE4
  • CARLA version: 0.9.13

Possible Drawbacks

We did complete tests and found no errors.


This change is Reviewable

@XiaoFei9704 XiaoFei9704 changed the title Supporting several new tags Supporting several additional OpenScenario 1.0 tags May 24, 2023
Copy link
Contributor

@glopezdiest glopezdiest left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 11 files at r1, 4 of 5 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SEEKERXC)


srunner/scenarioconfigs/openscenario_configuration.py line 372 at r3 (raw file):

                " Warning: The actor '%s' was not assigned an initial position. Using (0,0,0)", actor_name)
            # pylint: enable=line-too-long
            return False

Why is these needed? Won't it break the code if we return abool instead of an empty transform?


srunner/scenariomanager/scenarioatomics/atomic_behaviors.py line 1394 at r3 (raw file):

                        "log_level": "INFO"}
                }
                self.log.info(json.dumps(dic))

Is this self.log meant to be a debug tool? Cna we remove it?


srunner/tools/openscenario_parser.py line 253 at r3 (raw file):

    }

    osc_traffic_signal_phase = {}

Where are these osc_traffic_signal_phase and osc_traffic_signal_controller being filled? As far as i can see, they are always empty

@XiaoFei9704
Copy link
Contributor Author

Sorry for taking so long to reply.
We have fixed several issues over this time. I will close this PR and my colleague will submit it again.

@synkrotron
Copy link
Contributor

@glopezdiest We have created a new PR( #1014) and the code is updated for your questions, thanks for your reivew.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants