NEW: Add LocationSensor for Input System - #2469
Conversation
…om/Unity-Technologies/InputSystem into im-parity/isx-2227-location-sensor
ekcoh
left a comment
There was a problem hiding this comment.
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
|
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 It's the same for every command struct. So I'd let that module migration as a separate initiative. |
Pauliusd01
left a comment
There was a problem hiding this comment.
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
|
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(); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
but this API is not Editor only ? That's why it's confusing... oh well
ekcoh
left a comment
There was a problem hiding this comment.
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/locationDistanceThresholdare only pushed to native fromOnAdded. Changing either afterwards has no effect until someone callsResetConfiguration()by hand. ShouldOnChangere-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. statusreturningStoppedwhen 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) |
There was a problem hiding this comment.
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.
|
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
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
LocationSensordevice that exposes the device's GPS and lifecycle through the standard Input System surface:latitude,longitude,altitude,horizontalAccuracy,verticalAccuracy,timestamp.InputSystem.EnableDevice/DisableDevice+statusandisEnabledByUseras query properties.Configure(desiredAccuracyInMeters, updateDistanceInMeters)+ResetConfiguration(), with project defaultsInputSettings.locationAccuracy/locationDistanceThreshold(both 10, matching legacyStart()), 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 defaultsDocumentation Impact
LocationSensor+ controls +status/isEnabledByUser/Configure/ResetConfiguration,InputSettings.locationAccuracy/locationDistanceThreshold, threepubliccommand structs). All carry XML docs.Documentation~/corresponding-old-new-api.mdnow mapsInput.location->LocationSensorwith 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
CoreTests_Devices.cs): reading,status,isEnabledByUser, fallback path (no native impl),Configure,ResetConfiguration.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.
Stopped/falsewhere no native impl exists (editor/desktop), so no regression to existing sensors.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:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.