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

Added the sample for the 3d tiles layer #1662

Merged
merged 19 commits into from
Apr 4, 2024
Merged

Conversation

jingyili1023
Copy link
Collaborator

@jingyili1023 jingyili1023 commented Oct 31, 2023

Description

Add a new cpp sample for the 3d tiles layer.

Type of change

  • Bug fix
  • New sample implementation
  • Sample viewer enhancement
  • Other enhancement

Platforms tested on:

  • Windows
  • Android
  • Linux
  • macOS
  • iOS

Checklist

  • Runs and compiles on all active platforms as a standalone sample
  • Runs and compiles in the sample viewer(s)
  • Branch is up to date with the latest main/v.next
  • All merge conflicts have been resolved
  • Self-review of changes
  • There are no warnings related to changes
  • No unrelated changes have been made to any other code or project files
  • Code is commented with correct formatting (CTRL+i)
  • All variable and method names are camel case
  • There is no leftover commented code
  • Screenshots are correct size and display in description tab (500px by 500px, platform agnostic)
  • If adding a new sample, it is added to the sample viewer
  • Cherry-picked to Main branch (if applicable)

Copy link
Collaborator

@tanneryould tanneryould left a comment

Choose a reason for hiding this comment

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

Honestly I'm really impressed, this is really well done! There's a few finicky changes, plus some code cleanup that needs to be done, but on the whole you should be really proud of this!

Copy link
Collaborator

@tanneryould tanneryould left a comment

Choose a reason for hiding this comment

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

Random code cleanup. Try to avoid double line breaks and generally the first line in a method should not be a line break.

@ldanzinger
Copy link
Contributor

What's the status of this PR? Is it being redesigned to use services and remove the PE Data, since 200.4 will automatically use the required grid files?

@jingyili1023
Copy link
Collaborator Author

@ldanzinger I created this PR but was unable to merge it because the dataset I used had not been finalized. Ping has logged an issue for this in our backlog. @khajra can probably shed more light on this.

@jingyili1023
Copy link
Collaborator Author

@tanneryould, I just updated this PR to reflect the change on the sample design. Please do another round of review. Thank you!

Copy link
Collaborator

@tanneryould tanneryould 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, just need to remove a few things

@jingyili1023
Copy link
Collaborator Author

Thanks for spotting these redundancies. I got them removed.

@jingyili1023 jingyili1023 requested a review from tanneryould April 3, 2024 21:52
@jingyili1023 jingyili1023 requested a review from tanneryould April 3, 2024 22:50
Copy link
Collaborator

@tanneryould tanneryould left a comment

Choose a reason for hiding this comment

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

LGTM thank you!

@tanneryould tanneryould self-requested a review April 4, 2024 18:36
@jingyili1023 jingyili1023 merged commit 1d21ba7 into v.next Apr 4, 2024
1 check passed
@jingyili1023 jingyili1023 deleted the jli/add3dtileslayer branch April 4, 2024 21:26
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