Boat control abstraction

[daleeidd]

Moves boat control code out to its own component. This makes the boats more flexible to accepting different inputs like the new input system or nav mesh agents without having to modify the boat source.

The boats read from a Vector3 to get its desired input. It could be changed to a Vector2. But maybe a submersible could use the Y value?

There is an abstract class which components must inherit from. They need to write to the _Input property which is what the boats read.

I haven't added copyright headers or documentation yet. I'll wait for feedback on the proposal.

[moosichu]

Really like this change overall! Only minor feedback. Thanks @daleeidd !

[moosichu]

Do we want this? Maybe it would be better to require the field to be assigned-to explicitly and then print an error and disable this script if this problem is encountered? (I personally like to keep things simple, and having one way of initialising as opposed to two reflects that, but please feel free to disagree on this point and don't let this be a merge-blocker).

[daleeidd]

Good point. I think allowing null here will be useful. For example, perhaps using boat probes for a broken vessel. A null component could be created for that use case though. So maybe we could warn on validation instead?

[huwb]

Or should we automatically attach a player control if no control is present?

[huwb]

Maybe its a sensible default that can head off support requests.

[daleeidd]

We removed that functionality for the register input components (auto assign material). Also, the control classes live in Crest-Examples which users might opt not to import. I created a proposal with PR #497 that might help address this problem better.

[huwb]

True. thinking it through, i think it wasnt obvious to me that theres a good default choice of the ocean input material. i guess here in this case it comes down to how we feel about the default behaviour being to apply player controls. i'll defer to your judgement

[daleeidd]

I have changed this to use validation to warn users that the boat won't move anywhere instead of getting a component.

[moosichu]

Changes look good to me (once prefab cleanup and documentation is done)! I will let you decide what to do about a null _boatControl value. :)

[huwb]

I think have a vector3 is a good shout.

"Bias" referred to adding a fixed value to the players input. I don't really remember the exact case, maybe that functionality is not needed anymore. But i guess what is now the 'bias' input could just be called BoatInputFixed or BoatInputConstant?

Should we be accomodating of extending to the new input system in the future? Should BoatControlPlayer be called BoatControlPlayerLegacy, or something similar to leave space? Removing/renaming files can cause issues with asset store updates so i guess we want to be solid-ish with file/class names.

[daleeidd]

"Bias" referred to adding a fixed value to the players input. I don't really remember the exact case, maybe that functionality is not needed anymore. But i guess what is now the 'bias' input could just be called BoatInputFixed or BoatInputConstant?

Oh right. That makes sense. I have renamed it to BoatControlFixed. Users can always create their own easily now if they want that use case.

Should we be accomodating of extending to the new input system in the future? Should BoatControlPlayer be called BoatControlPlayerLegacy, or something similar to leave space?

That's a good idea. I thought we might be able to use the defines to accommodate both. It is possible, but the new system is different enough that we should create a seperate component for it. I've renamed it to BoatControlPlayerLegacy.

[daleeidd]

Looks like we can just support the new input system in a similar way to how we are doing it already. I have reverted the rename. I'll just add support directly to this component so it will support both. I'll do it in a new PR since we need to also support it wherever we read from the old input system (DemoCamera, OceanDebugGUI etc) or it will error.

[huwb]

Ok if it works directly into that component using defines (?) then that sound good

[daleeidd]

It has been a while but I resolved the last bit of feedback by using validation instead of getting a component when the boat control not provided by the user.

I added documentation. And I redid the prefabs since it was easier to bring master in. There is some noise in the prefab diff but it is minor. I cleaned most of the big stuff out.

I managed to get it working with Unity navigation. I will do a separate PR for that.

[ghost]

I think abstraction would be beneficial in providing a more robust foundation for future additions but I wouldn't say it's a high priority to implement if it still needs additional work.

The exception being support for the input system package [ #808 ].

It might also be useful to consider widening the scope of what can be changed via inspector in the future (for example, the debug draw line colours used are hard coded)

[huwb]

It's been a while since I've looked at this but reviewing again my feeling is that we should auto assign a player control component if no control is present, otherwise it's going to trip people up and generate support traffic. Maybe an option is to auto assign the control if null and generate a warning when it does it, in addition to the validation warning you added. Would that work?

[ghost]

It's been a while since I've looked at this but reviewing again my feeling is that we should auto assign a player control component if no control is present, otherwise it's going to trip people up and generate support traffic. Maybe an option is to auto assign the control if null and generate a warning when it does it, in addition to the validation warning you added. Would that work?

What happens in the case of a user who removes the player control component expecting their boat to have physics but not to be controllable?

Should an example script exist that can be assigned by the player that is basically blank/doesn't allow control of a boat (for example, maybe to simulate a boat that is dead in the water as mentioned by Dale) or are users who don't want a player controller component automatically assigned expected to remove the code that performs that check?

[huwb]

Fair point. Hm bit of a dilemma. Guess its fine as is then.

[daleeidd]

It's been a while since I've looked at this but reviewing again my feeling is that we should auto assign a player control component if no control is present, otherwise it's going to trip people up and generate support traffic. Maybe an option is to auto assign the control if null and generate a warning when it does it, in addition to the validation warning you added. Would that work?

We could add the control in the Reset method?

[huwb]

Reset can be called at any time by the user so i'm not certain if that makes sense.

I reckon we should merge this as is. What is outstanding (besides the conflicts)?

[daleeidd]

Reset can be called at any time by the user so i'm not certain if that makes sense.

True.

I reckon we should merge this as is. What is outstanding (besides the conflicts)?

Sounds good. Documentation. And I will add a fix function. And maybe merge in #826 first and then I can deal with porting it over.