Skip to content

NEW: Add LocationSensor for Input System - #2469

Open
MorganHoarau wants to merge 15 commits into
im-parity/stagingfrom
im-parity/isx-2227-location-sensor
Open

NEW: Add LocationSensor for Input System#2469
MorganHoarau wants to merge 15 commits into
im-parity/stagingfrom
im-parity/isx-2227-location-sensor

Conversation

@MorganHoarau

@MorganHoarau MorganHoarau commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Description

Addresses ISX-2227 is part of the Input Manager parity epic ISX-2108.

This PR is the managed layer only. Refer to the native bridge PR here.

Add a LocationSensor device that exposes the device's GPS and lifecycle through the standard Input System surface:

  • Readings as event-driven controls: latitude, longitude, altitude, horizontalAccuracy, verticalAccuracy, timestamp.
  • Lifecycle via InputSystem.EnableDevice / DisableDevice + status and isEnabledByUser as query properties.
  • Configure(desiredAccuracyInMeters, updateDistanceInMeters) + ResetConfiguration(), with project defaults InputSettings.locationAccuracy / locationDistanceThreshold (both 10, matching legacy Start()), surfaced in Project Settings.

Backed by the shared native LocationService.

Class diagram: LocationSensor and command surface
classDiagram
    class Sensor
    class LocationSensor {
        +AxisControl latitude
        +AxisControl longitude
        +AxisControl altitude
        +AxisControl horizontalAccuracy
        +AxisControl verticalAccuracy
        +DoubleControl timestamp
        +LocationServiceStatus status
        +bool isEnabledByUser
        +static LocationSensor current
        +Configure(float, float)
        +ResetConfiguration()
    }
    class LocationState {
        <<internal, IInputStateTypeInfo>>
        +double timestamp
        +float latitude
        +float longitude
        +float altitude
        +float horizontalAccuracy
        +float verticalAccuracy
    }
    class InputSettings {
        +float locationAccuracy
        +float locationDistanceThreshold
    }
    class QueryLocationStatusCommand
    class QueryLocationEnabledByUserCommand
    class ConfigureLocationCommand

    Sensor <|-- LocationSensor
    LocationSensor ..> LocationState : stateType
    LocationSensor ..> QueryLocationStatusCommand : status (LSTA)
    LocationSensor ..> QueryLocationEnabledByUserCommand : isEnabledByUser (LUSR)
    LocationSensor ..> ConfigureLocationCommand : Configure (LCFG)
    LocationSensor ..> InputSettings : ResetConfiguration reads defaults
Loading

Documentation Impact

  • Public API added (LocationSensor + controls + status / isEnabledByUser / Configure / ResetConfiguration, InputSettings.locationAccuracy / locationDistanceThreshold, three public command structs). All carry XML docs.
  • Migration table updated: Documentation~/corresponding-old-new-api.md now maps Input.location -> LocationSensor with a usage snippet.

Testing status & QA

Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.

ℹ️ PLEASE REFER TO: https://github.cds.internal.unity3d.com/morgan-hoarau/internal-input-manual-test-projects/pull/2

  • 6 unit tests added (CoreTests_Devices.cs): reading, status, isEnabledByUser, fallback path (no native impl), Configure, ResetConfiguration.
  • On-device (via the InputFeatureParity Validation project):
    • Tested on Android, iOS and Web (Windows, Mac, Android and iOS) in both input backend "Both" and "Input System (new)".

Overall Product Risks

Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.

  • Complexity: 1
    • Additive, self-contained device; fallback to Stopped / false where no native impl exists (editor/desktop), so no regression to existing sensors.
  • Halo Effect: 1
    • Touches shared InputSettings (two new serialized fields + settings UI).

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

@MorganHoarau MorganHoarau changed the title [DRAFT] Im parity/isx 2227 location sensor NEW: Add LocationSensor for Input System Aug 11, 2026
Comment thread Packages/com.unity.inputsystem/InputSystem/Runtime/Devices/Sensor.cs Outdated
@MorganHoarau
MorganHoarau marked this pull request as ready for review August 11, 2026 15:39

@u-pr u-pr Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Harness Review

LGTM

The new sensor implementation generally aligns with surrounding sensor, command, and settings patterns, while the associated migration guidance needs correction.

Reviewed commit 4c9aef3

🤖 Helpful? 👍/👎

Comment thread Packages/com.unity.inputsystem/Documentation~/corresponding-old-new-api.md Outdated

@ekcoh ekcoh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only glanced at this PR so far but my main reaction is that Command types are defined in package instead of being internal types within module. I would strongly suggest placing them in module instead. If there are issues with that which I have overlooked I am happy to discuss to find a good way forward.

Exposing them like this basically makes internal ABI part of the API which is something I think we would benefit to move away from

@MorganHoarau

Copy link
Copy Markdown
Collaborator Author

Made the 3 location command structs internal, so the native ABI is no longer public API.

However, moving them into the module is blocked by dependency as they're built on InputDeviceCommand / InputDeviceCommandInfo / FourCC, which live in the package, and the module can't reference the package.

It's the same for every command struct. So I'd let that module migration as a separate initiative.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Pauliusd01 Pauliusd01 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've only been asked to review the testing already done by Morgan, I've looked at his project before and the testing coverage seemed sufficient to me

@todi1856

Copy link
Copy Markdown
Member

Could you fix the link in the description "This PR is the managed layer only. Refer to the native bridge PR here." "here" doesn't point to anything ?

if (m_LocationAccuracy == value)
return;
m_LocationAccuracy = Mathf.Max(0f, value);
OnChange();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this API callable at runtime? if so, who's reponsible for calling

            var command = ConfigureLocationCommand.Create(desiredAccuracyInMeters, updateDistanceInMeters);
            ExecuteCommand(ref command);

to update this settings on the native side ?

the same for locationDistanceThreshold ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've provided a new LocationSensor.Configure that is responsible for calling it. It will restart the service on the spot too.

It replaces all the overrides of IM LocationService.Start.

Does that sounds reasonable to you?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I still don't get it a bit, is there a scenario, where me as user could could call setters for InputSettings.locationAccuracy, or InputSettings.locationDistanceThreshold at runtime (on Android device) ? If so, what exactly would that to if I would call these ?

@MorganHoarau MorganHoarau Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No, these values are editor set only, they are used as default value when enabling the Location sensor for the first time (through Configure itself)

At this line.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but this API is not Editor only ? That's why it's confusing... oh well

@ekcoh ekcoh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed alongside the native PR. The managed side is in good shape — introducing InputSystem.LocationServiceStatus to break the legacy module dependency was the right call, and the command structs and test coverage for the query/configure paths are solid. Thanks for making the commands internal in 2a473d8, that matches the sibling sensor states.

On my earlier point about interop types, the visibility half landed and thanks for that. On placement you're right that the command structs can't move: they embed InputDeviceCommand, implement IInputDeviceCommandInfo and expose FourCC, and module-to-package is the wrong direction. Same for LocationState with [InputControl] and IInputStateTypeInfo. I'll drop that part.

But I'd separate the envelope from the field types inside it. LocationServiceStatus (enum) has no package types in it at all, and the same now exists in Runtime/Input/LocationService.h, in UnityEngine.LocationServiceStatus, and here, held together only by comments. A package struct can take a module-owned field type, and ISX already consumes several runtime files from module. So QueryLocationStatusCommand could stay put with its status field typed as a module-owned internal enum, and native could static_assert its half in the associated sensor device translation unit.

I realise that rubs against the "only NativeInputRuntime.cs should reference UnityEngineInternal.Input" convention, so it's worth a conversation rather than a drive-by change. The reason I keep pushing is that this enum is the one place where drift is silent. A wrong FourCC or struct size fails immediately and loudly, whereas a status enum that drifts just reports Running when the service actually failed.

A few smaller notes:

  • locationAccuracy / locationDistanceThreshold are only pushed to native from OnAdded. Changing either afterwards has no effect until someone calls ResetConfiguration() by hand. Should OnChange re-push to the device?
  • Nothing covers enable/disable mapping to ENBL/DSBL, which is the path that actually starts and stops the service. Worth a test given the native side keys its own enabled state off it.
  • status returning Stopped when the command fails conflates "no native backend" with "service is stopped". It's called out in the comment, just noting it's observable from the public API.

/// If the sensor is already enabled, readings may briefly pause while they are applied.
/// If the sensor is disabled, values apply when the device is enabled.
/// </remarks>
public void Configure(float desiredAccuracyInMeters, float updateDistanceInMeters)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Related to a comment on the native PR: on iOS the ENBL and LCFG paths end up constructing a CLLocationManager, which Core Location requires to happen on a thread with an active run loop. Off the main thread the delegate callbacks never fire, and isEnabledByUser gets stuck at false permanently with nothing to indicate why.

InputSystem.cs:49 already says the API is main-thread-only with a few listed exceptions, so this isn't a new rule — but LocationSensor is stricter than the native contract it sits on (InputDeviceIOCTL explicitly permits parallel execution for the same callback), and right now nothing on either side records that.

Could Configure, ResetConfiguration, status and isEnabledByUser document the requirement on their <remarks>, and assert main-thread before issuing the command? A failure here is silent and permanent, which is the case where an assert earns its keep — the alternative is a user reporting "location never works" with no way to trace it back.

@ekcoh

ekcoh commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

A follow-up based on my previous feedback as well: currently it seems there is no type dependency from package to module. While this makes CI happy atm it also hides a real dependency. If e.g. the enum would come from module, the package also need a version based asmdef define to enable the code selectively here in this decoupled package. While a bit more work I think that is fine since it makes things more robust when type sits in the correct place. It could of course also be decided to handle it later as part of a bigger refactor for this anti-pattern at a later stage as long as its internal.

also mention of feature capability check
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.

4 participants