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

Refactor TransportManager #1320

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@zaklaus
Copy link

commented Jun 6, 2019

Implementation of a horse mount (or dismount when already on a horse/cart) for G key. This required some changes to the TransportManager to make sure transportation change conditions are kept in sync via a public API. ToggleMount() can easily mount or dismount a horse if conditions are met.

I can optionally get rid of the G key shortcut, if wanted.

NOTE:
Call to ToggleMount() works even inside of buildings or dungeons. This behaviour was kept intentionally to allow mods to use mounts even inside interiors. Of course, the front-end logic defaults to classic behaviour and only allows you to toggle mount in the exterior.

Introduced an option in the Enhancements section called HorseMountShortcut.

@petchema

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

They're forward and backward KeyBind.txt compatibility issues with this patch, which can be a problem because it's a settings file (hence shared among all instances, and not cleaned by reinstalling DFU):

  • If you had something already bound to G key, bindings play musical chairs, each one getting its default binding (I think) until everything is assigned a separate key;
  • Either way, in keybindings setup screen the G binding appears in front of "Steal mode", right below "Transport". All bindings after that are shifted one place;
  • If a KeyBind.txt with the new horse mount keybind is read by a version of DFU that doesn't have that feature, all bindings are lost (no bindings in KeyBind.txt, everything bound to "None" in setup).
@zaklaus

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Thanks for the report, I will adjust the changes accordingly.

EDIT:
I've addressed the 2nd issue locally. I'm not sure how to proceed with the 1st or 3rd issue.

In case of the 3rd issue, I'm not sure how to make sure the existing instances understand that a newly introduced keybinding is to be skipped (other than by patching this issue in the current version, so it won't occur in the future)

  1. I see that we deserialize Dictionary<KeyCode, Actions> from JSON, however I'm unsure how to handle the possibility that an Action doesn't exist in the current build. I'd find it easier to store actions as strings, so that we could perform resolution during the load and simply skip unknown actions.

As for the 1st issue, I'm not sure how to make sure user-defined keybindings won't collide after a new update. Ideally, if a collision happens, the subsequent colliding actions should get unassigned. However, once again, this fix would only apply to the current version.

  1. I see we already make sure duplicate keys for various actions get handled FIFO style, however due to the nature of the serialized data (hashtable) you technically can't have 2 same keys in the collection. However, I believe that if player already had a G key assigned to a specific action, MountHorse would simply be unbound and won't even be stored in the settings file.

@zaklaus zaklaus changed the title Refactor TransportManager and implement G key for toggling a mount [DON'T MERGE] Refactor TransportManager and implement G key for toggling a mount Jun 6, 2019

@zaklaus zaklaus changed the title [DON'T MERGE] Refactor TransportManager and implement G key for toggling a mount Refactor TransportManager and implement G key for toggling a mount Jun 6, 2019

@ajrb

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

I think this is probably mod territory since there are no plans to expand the keybindings screen in DFU.

@zaklaus

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

I've edited the InputManager to support these issues better in the future. Unknown actions will be ignored (and saved on exit). As for the MountHorse particularly, I believe it could be moved onto a mod territory, in that case I'd still recommend cherry-picking TransportManager changes, so that these
related actions will be kept in sync across the codebase.

EDIT: I won't be available for a couple of days, so I'll let this be open.

@zaklaus zaklaus force-pushed the zaklaus:master branch from f20fab7 to 049f26c Jun 6, 2019

zaklaus added some commits Jun 6, 2019

Refactor TransportManager
This required some changes to the TransportManager to make sure transportation change conditions are kept in sync via a public API. ToggleHorse() can easily mount or dismount a horse if conditions are met.
Rename ToggleHorse method
# Conflicts:
#	Assets/Scripts/Game/TransportManager.cs

@zaklaus zaklaus force-pushed the zaklaus:master branch from 02e9127 to 6b8cba5 Jun 6, 2019

@zaklaus

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

I've separated PR changes into 3 branches. This PR now only contains changes made to the TransportManager and relevant facilities. #1322 contains InputManager changes as a separate PR. Mount toggle action will be kept in a separate branch in case of mod interest.

@zaklaus zaklaus changed the title Refactor TransportManager and implement G key for toggling a mount Refactor TransportManager Jun 6, 2019

Fix small code issues
These issues were caused by wrong git manipulation.

@zaklaus zaklaus force-pushed the zaklaus:master branch from 602d5aa to 0c5151a Jun 7, 2019

Rename ToggleHorse method
# Conflicts:
#	Assets/Scripts/Game/TransportManager.cs

# Conflicts:
#	Assets/Scripts/Game/TransportManager.cs
@zaklaus

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

Once I return home, I would share the toggle mount mod, which would make use of this PR specifically.

EDIT: Here is the toggle mount mod. It makes use of the PR changes, allows you to change the hotkey and respects classic rules.
toggle mount.zip

@Interkarma

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

Hello @zaklaus! :)

Thank you for your work here, but I'm not sure why the changes are necessary. It seems possible to create all of this functionality in a mod without refactoring TransportManager. Could you please outline the core issue that requires these changes?

@zaklaus

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

Hi,

I originally wanted to create a simple toggle horse mod on a hotkey, however I've noticed that there wasn't an API for checking whether the horse is available. I've noticed the UI logic directly accesses the player's inventory to find out if he owns a special item (horse), which in turn would enable the horse button in the transport menu.

Since I needed to do the same check on the mod side, I found it redundant and easier to just present an API for easily checking whether the player owns a horse/cart with a simple method to easily change to either of those modes (ToggleMount()).

It is possible this isn't really required, since the core will probably never deal with transportation mode changes outside of the UI logic (T menu), but having an API to check for requirements instead of accessing the inventory directly to figure out if I can saddle a horse just felt cleaner.

@Interkarma

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

Thank you, I understand where you're coming from now. I'm happy for TransportManager to expose the HasHorse, etc. helpers. And while ToggleMount() is rather specific to your mod, it's a simple helper and I don't mind it being there. I also really appreciate that you trimmed this one down earlier after feedback from the others. Will test and merge this one in next few days.

@zaklaus

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

Thank you, it was a nice experience exploring the codebase.

@bansheexyz

This comment has been minimized.

Copy link

commented Jun 22, 2019

Just wanted to add that I personally like your idea here, zak. Toggling between foot and horse with "T" instead of bringing up a menu seems like it would work well. You could force the cart-drawn horse if a cart is owned, and then obviously do ship when standing in ocean water. I wonder why classic thought a menu was necessary? Anyway, fairly simple mod that saves a click, not up to me but seems like a good stock option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.