Skip to content

Add proj4 string to GroundTruth fixes #276 #275

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
Merged
3 commits merged into from Nov 5, 2018
Merged

Add proj4 string to GroundTruth fixes #276 #275

3 commits merged into from Nov 5, 2018

Conversation

@ghost
Copy link

@ghost ghost commented Oct 1, 2018

Using field number 16 because the projection string will not be a small field, so 14,15 can be used in future releases for repeated or smaller fields.

@ghost ghost requested review from a user and pmai October 2, 2018 07:34
@ghost ghost mentioned this pull request Oct 2, 2018
Closed
@ghost ghost added FeatureRequest Proposals which enhance the interface or add additional features. Suggestions I just want to drop by and leave this suggestion to think about. labels Oct 2, 2018
@ghost ghost added this to the v3.1.0 milestone Oct 2, 2018
@ghost ghost changed the title Add proj4 string to GroundTruth Add proj4 string to GroundTruth solves #276 Oct 2, 2018
@ghost ghost changed the title Add proj4 string to GroundTruth solves #276 Add proj4 string to GroundTruth fixes #276 Oct 2, 2018
ghost
ghost approved these changes Oct 23, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks well defined for me!

@ghost
Copy link
Author

ghost commented Oct 23, 2018

@pmai would you expect this in the sensor view or the ground truth?

@pmai
Copy link
Contributor

pmai commented Oct 23, 2018

I think groundtruth is the proper place, or it might in the future be something to be moved to setup messages if it’s constant over time.

@ghost
Copy link
Author

ghost commented Oct 26, 2018

@pmai and @CarloVanDriestenBMW: What do you think about the field number? Does it make sense to choose a non-consecutive value due to performance reasons? I'd like to finish this PR in order to be able to discuss new ones (i.e. #277)

@pmai
Copy link
Contributor

pmai commented Oct 29, 2018

I don't think we should think too much about performance for this, but generally speaking this PR could be changed to use id 14, since 16 does not seem to buy us anything...

@ghost
Copy link
Author

ghost commented Nov 5, 2018

Reserving field numbers below 16 for frequently used fields is recommended by the protobuf language guide due to the resulting smaller byte size of the message.
But using consecutive numbers is also fine for me.

@ghost ghost merged commit 92fdf06 into OpenSimulationInterface:master Nov 5, 2018
@ghost ghost deleted the projection_string_to_gt branch November 5, 2018 13:47
@ghost ghost mentioned this pull request Nov 20, 2018
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pmai pmai Awaiting requested review from pmai

Assignees

No one assigned

Labels

FeatureRequest Proposals which enhance the interface or add additional features. Suggestions I just want to drop by and leave this suggestion to think about.

Projects

None yet

Milestone

v3.1.0

Development

Successfully merging this pull request may close these issues.

1 participant

@pmai