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

[New] Show viewshed from point in scene #70

Merged
merged 22 commits into from
Oct 11, 2022
Merged

[New] Show viewshed from point in scene #70

merged 22 commits into from
Oct 11, 2022

Conversation

yo1995
Copy link
Collaborator

@yo1995 yo1995 commented Sep 28, 2022 •
edited

Description

This PR implements Show viewshed from point in scene in Analysis category.

URL to README: https://github.com/ArcGIS/arcgis-runtime-samples-swift/tree/Ting/New-ViewshedPoint/Shared/Samples/Show%20viewshed%20from%20point%20in%20scene

Linked Issue(s)

  • swift/issues/2557

How To Test

Please notice: you may not be able to see the toolbar button on iOS 16 simulators. This is a known issue and may be related to the new NavigationStack APIs. Test on iOS 15 to see the expected behavior.

  • Run the sample
  • Try out each setting
  • Tap on the scene view
  • See if the viewshed is expected

Screenshots

220928-viewshed-demo.mp4

To Discuss

@yo1995 yo1995 self-assigned this Sep 28, 2022
@yo1995 yo1995 changed the base branch from main to v.next September 28, 2022 23:26
@yo1995 yo1995 marked this pull request as ready for review September 29, 2022 04:31
vquach2404
vquach2404 reviewed Sep 30, 2022
View reviewed changes
Copy link
Contributor

@vquach2404 vquach2404 left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few suggestions.

Shared/Samples/Show viewshed from point in scene/ShowViewshedFromPointInSceneView.Model.swift Outdated Show resolved Hide resolved
...how viewshed from point in scene/ShowViewshedFromPointInSceneView.ViewshedSettingsView.swift Outdated Show resolved Hide resolved
Shared/Samples/Show viewshed from point in scene/ShowViewshedFromPointInSceneView.swift Outdated Show resolved Hide resolved
Shared/Samples/Show viewshed from point in scene/ShowViewshedFromPointInSceneView.Model.swift Outdated Show resolved Hide resolved
@yo1995 @vquach2404
Apply suggestions from code review.
6a34f4f 
Co-authored-by: Vivian Quach <viv10382@esri.com>
@yo1995 yo1995 requested review from philium and removed request for zkline101 October 4, 2022 16:59
@yo1995 yo1995 added the low priority No rush to review this PR label Oct 4, 2022
@philium
Copy link
Contributor

philium commented Oct 5, 2022

Also, the Settings button is missing when run on an iPhone 14 Pro Max simulator:

image

I'm not sure why.

@yo1995
Copy link
Collaborator Author

yo1995 commented Oct 5, 2022 •
edited

Also, the Settings button is missing when run on an iPhone 14 Pro Max simulator:

See the How to test note at the top. It seems to be either an iOS 16 issue or a NavigationView (deprecated in iOS 16) issue. Similar discussion: https://developer.apple.com/forums/thread/712484
I also tried with Xcode 14.1 beta 3 and iOS 16.1, but it still doesn't show the button.

I've a card logged for navigation view improvements, but haven't got to it yet.

@yo1995 yo1995 requested a review from philium October 5, 2022 21:57
@philium
Copy link
Contributor

philium commented Oct 6, 2022

See the How to test note at the top.

I'm sorry. I completely missed that!

yo1995 and others added 2 commits October 6, 2022 14:54
@yo1995 @philium
Apply suggestions from code review
33579f7 
Co-authored-by: Philip Ridgeway <philip.ridgeway@gmail.com>
@yo1995
Apply suggestions from code review.
f5f5891 
See:
- #70 (comment)
- #70 (comment)
@yo1995 yo1995 requested a review from philium October 6, 2022 22:22
philium
philium previously approved these changes Oct 7, 2022
View reviewed changes
dfeinzimer
dfeinzimer reviewed Oct 10, 2022
View reviewed changes
Copy link
Contributor

@dfeinzimer dfeinzimer left a comment •
edited

Choose a reason for hiding this comment

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

I'm getting a black screen in the simulator. Is a license required for this sample?
It was my secrets file

Shared/Samples/Show viewshed from point in scene/README.md Outdated Show resolved Hide resolved
Shared/Samples/Show viewshed from point in scene/ShowViewshedFromPointInSceneView.Model.swift Outdated Show resolved Hide resolved
dfeinzimer
dfeinzimer reviewed Oct 10, 2022
View reviewed changes
Copy link
Contributor

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

Just the one suggestion

Shared/Samples/Show viewshed from point in scene/ShowViewshedFromPointInSceneView.Model.swift Outdated Show resolved Hide resolved
vquach2404
vquach2404 approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@vquach2404 vquach2404 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!

@yo1995 yo1995 merged commit 13d967a into v.next Oct 11, 2022
@yo1995 yo1995 deleted the Ting/New-ViewshedPoint branch October 11, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philium philium philium left review comments

@dfeinzimer dfeinzimer dfeinzimer approved these changes

@vquach2404 vquach2404 vquach2404 approved these changes

Assignees

@yo1995 yo1995

Labels
low priority No rush to review this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yo1995 @philium @dfeinzimer @vquach2404