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

New Sample: Overview Map Sample #164

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Conversation

mmgaw113
Copy link
Collaborator

@mmgaw113 mmgaw113 commented Dec 5, 2024

Sample

The overview map sample is a highly requested sample and was given moderate priority from Adrien.
The sample builds off the overview map created in for the AR Feature Layer sample currently under review. It uses two map components each with a different basemap. The Main ArcGIS Map has a 3D object scene layer where as the overview does not.

Checklist

  • PR title follows convention - keyword: Short description of change
  • PR targets the correct branch
  • Self-review of changes
  • There are no warnings related to changes
  • A build was made and tested on all relevant platforms
  • No unrelated changes have been made to any other code or project files
  • No unnecessary includes or namespaces added
  • Code follows plugin coding style
  • New code and changed code has proper formatting
  • No unintentional formatting changes
  • Commits have descriptive titles

ArcGIS Maps SDK Version

Maps SDK 1.7 W/ Unity 2022.3 LTS

@mmgaw113 mmgaw113 requested a review from a team December 5, 2024 21:51
@mmgaw113 mmgaw113 self-assigned this Dec 5, 2024
@Jade-JadeH
Copy link
Collaborator

  1. Seeing a lighting issue

image

  1. Switching between HDRP and URP doesn't work in the sample viewer

Copy link
Collaborator

@Jade-JadeH Jade-JadeH left a comment

Choose a reason for hiding this comment

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

See the comments above

@mmgaw113
Copy link
Collaborator Author

mmgaw113 commented Dec 5, 2024

Pull the recent changes. I noticed it too and already have it fixed.

@mmgaw113
Copy link
Collaborator Author

mmgaw113 commented Dec 5, 2024

Screen.Recording.2024-12-05.at.3.15.31.PM.mov

@Jade-JadeH
Copy link
Collaborator

Pull the recent changes. I noticed it too and already have it fixed.

I specifically pulled it after you pushed the changes.

@mmgaw113 mmgaw113 requested a review from Jade-JadeH December 5, 2024 23:18
@mmgaw113
Copy link
Collaborator Author

mmgaw113 commented Dec 5, 2024

I just tested it and its working, so it sounds like you might have missed the latest commit. See video of it working above ^

@Jade-JadeH
Copy link
Collaborator

Jade-JadeH commented Dec 6, 2024

I like the overall UI design and UX. Works really well.

The two issues I pointed out previously have been resolved after the latest commits.

  1. Could you have collider on the top arcgismap or make the second arcgismap invisible, so that when I fly through the map I don't see two maps?
Unity_xhtjrbAu6Y.mp4
  1. In doc we have a review process which checks for grammar and punctuation so I took a cursory look here.

@mmgaw113
Copy link
Collaborator Author

mmgaw113 commented Dec 6, 2024

Could you have collider on the top arcgismap or make the second arcgismap invisible, so that when I fly through the map I don't see two maps?

Mesh colliders are enabled... Looks like a bug with mesh colliders because you can go through but once you through to the map it won't let you go any further.

@avr757
Copy link
Collaborator

avr757 commented Dec 6, 2024

I think the UI/UX looks good with the sample! I only have a few small things on the sample viewer menu during testing,

  1. It looks like we are only capitalizing the first letter in unity sample list, so let's make those highlighted letter smaller case just to be consistent with other sample names.
    image

  2. For some reason, I noticed the entire sample list was shifted to the left, can you make a quick change on that?
    image

    The scroll view position should be x=375
    image

    Should look like this:
    image

@mmgaw113 mmgaw113 requested a review from Jade-JadeH December 6, 2024 16:56
Copy link
Collaborator

@avr757 avr757 left a comment

Choose a reason for hiding this comment

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

Ui/UX comments are addressed, looks good now! Thanks

private ArcGISCameraControllerComponent cameraController;
private ArcGISLocationComponent cameraLocationComponent;
private ArcGISLocationComponent locationComponent;
private float northOffset = 225.0f;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to offset the rotation? Why 5/8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The orientation of the location marker is wrong. Without an offset it will be facing the wrong direction. 225 is the value it is off by. Only way to get rid of the offset is to fix it in a 3D modeling software like Blender

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you're not planning to expose or change this, it would probably be better as const

@Jade-JadeH
Copy link
Collaborator

We've been using camel case so please change naming like the one below

image

@Jade-JadeH
Copy link
Collaborator

Mesh colliders are enabled... Looks like a bug with mesh colliders because you can go through but once you through to the map it won't let you go any further.

Is this issue logged?

@mmgaw113
Copy link
Collaborator Author

mmgaw113 commented Dec 6, 2024

We've been using camel case so please change naming like the one below

image

We don't have a specific naming convention for sub-folders within a sample's folder. But looking at other samples, they follow Pascal Naming Convention, so this is appropriate and I will not be changing it. See attached weather query folder structure.
Screenshot 2024-12-06 at 2 05 10 PM

@Jade-JadeH
Copy link
Collaborator

What I was referring to is to remove the space in "UI element" and change it to "UIElement" as space is prone to errors in compiling. Some call it camel case, some call it upper camel case, some call it pascal case

@mmgaw113
Copy link
Collaborator Author

mmgaw113 commented Dec 6, 2024

@ZackAllen are we going to start enforcing naming convention for sub folders? Or just Sample Parent Folders?

@mmgaw113 mmgaw113 requested a review from ZackAllen December 9, 2024 16:26
private Volume skyAndFog;
private Exposure exposure;

void Start()
Copy link
Collaborator

Choose a reason for hiding this comment

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

private void start

exposure.active = false;
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

EOF

overviewCamera.orthographicSize = (float)cameraLocationComponent.Position.Z;
SetLocationMarkerScale();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

EOF

private ArcGISCameraControllerComponent cameraController;
private ArcGISLocationComponent cameraLocationComponent;
private ArcGISLocationComponent locationComponent;
private float northOffset = 225.0f;
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you're not planning to expose or change this, it would probably be better as const

});
}

private void Update()
Copy link
Collaborator

Choose a reason for hiding this comment

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

These chunks are nearly the same and can be refactored. Here's a possible refactor:

private void Update()
    {
        if (cameraLocationComponent == null || locationComponent == null || mapComponent == null)
        {
            return;
        }

        UpdateLocationAndRotation();

        if (rotationToggle.isOn)
        {
            overviewMap.transform.localEulerAngles = new Vector3(0, 0, (float)cameraLocationComponent.Rotation.Heading);
        }
    }

    private void UpdateLocationAndRotation()
    {
        var cameraPosition = cameraLocationComponent.Position;
        var cameraRotation = cameraLocationComponent.Rotation;

        var newPosition = new ArcGISPoint(cameraPosition.X, cameraPosition.Y, locationComponent.Position.Z, mapComponent.OriginPosition.SpatialReference);
        locationComponent.Position = newPosition;

        var newRotation = new ArcGISRotation(cameraRotation.Heading + northOffset, locationComponent.Rotation.Pitch, locationComponent.Rotation.Roll);
        locationComponent.Rotation = newRotation;

        var cameraComponentLocation = cameraComponent.GetComponent<ArcGISLocationComponent>();
        if (cameraComponentLocation != null)
        {
            cameraComponentLocation.Position = new ArcGISPoint(cameraPosition.X, cameraPosition.Y, cameraComponentLocation.Position.Z, mapComponent.OriginPosition.SpatialReference);
        }
    }

@mmgaw113 mmgaw113 marked this pull request as draft January 6, 2025 19:27
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.

5 participants