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

Boat control abstraction #486

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Boat control abstraction #486

wants to merge 9 commits into from

Conversation

daleeidd
Copy link
Collaborator

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.

Copy link
Contributor

@moosichu moosichu left a comment

Choose a reason for hiding this comment

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

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


if (!_boatControl)
{
_boatControl = GetComponent<BoatControl>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@huwb huwb May 6, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@daleeidd daleeidd Aug 19, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@moosichu moosichu left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

huwb commented May 2, 2020

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
Copy link
Collaborator Author

daleeidd commented May 3, 2020

"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
Copy link
Collaborator Author

daleeidd commented May 4, 2020

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
Copy link
Contributor

huwb commented May 6, 2020

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

@daleeidd
Copy link
Collaborator Author

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
Copy link

ghost commented Apr 24, 2021

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
Copy link
Contributor

huwb commented Apr 24, 2021

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
Copy link

ghost commented Apr 25, 2021

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
Copy link
Contributor

huwb commented Apr 25, 2021

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

@daleeidd
Copy link
Collaborator Author

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
Copy link
Contributor

huwb commented May 12, 2021

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
Copy link
Collaborator Author

daleeidd commented May 12, 2021

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.

@daleeidd daleeidd added this to the Next Release milestone Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants