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

Vehicle type exclusion list for "Disable despawning" feature #1441

Merged
merged 12 commits into from
Mar 6, 2022

Conversation

krzychu124
Copy link
Member

Add options to allow despawn of specific vehicle types when "Disable despawning" feature is active

Closes #1434

@krzychu124 krzychu124 added feature A new distinct feature DESPAWN TOOLS Feature: Clear traffic, toggle despawn, vehicle/parked/cim despawns labels Feb 28, 2022
@krzychu124 krzychu124 self-assigned this Feb 28, 2022
…nto feature/allow-despawn-by-type

� Conflicts:
�	TLM/TLM/TLM.csproj
@krzychu124 krzychu124 marked this pull request as ready for review February 28, 2022 20:11
@krzychu124 krzychu124 requested review from originalfoo, DaEgi01 and kvakvs and removed request for originalfoo and DaEgi01 February 28, 2022 20:11
Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

Still need to test in-game

@originalfoo originalfoo added this to the 11.6.5.1 milestone Feb 28, 2022
@originalfoo
Copy link
Member

Tested in-game and it works but... it's a bit weird. Summarising the MayDespawn logic:

may despawn =

    Despawning is not disabled, or
    Vehicle is blown|floating, or
    Vehicle is parking, or
    Vehicle type is allowed to despawn

I have no way of knowing, as a user, that if I don't enable Disable Despawning then my disabled Allow Despawning filters won't have any effect.

Additionally, the options are on separate tabs (Gameplay vs. Maintenance) and there's no obvious linkage between the TM:PE Disable Despawning toolbar button and the filters in mod options.

I'm wondering if it would be better to refactor how despawning works more generally? For example:

  • Move the filters to a pop-up panel
  • Right-click on the Disable Despawning toolbar button = open panel
  • User chooses which vehicle types are allowed (disallowed?) to despawn
  • Left clicking the toolbar button works as it currently does, toggling the main despawn option

@krzychu124
Copy link
Member Author

@aubergine10 what do you think?
image

@originalfoo
Copy link
Member

Looks good!

…s or via right click on despawning button on the main menu in game
@krzychu124
Copy link
Member Author

Updated.
Also, I think we could change that if..else for missing translation keys to cover Stable and TEST till we add missing options (as translation keys are not final yet).

@originalfoo originalfoo mentioned this pull request Mar 1, 2022
@originalfoo
Copy link
Member

Also, I think we could change that if..else for missing translation keys to cover Stable and TEST till we add missing options (as translation keys are not final yet).

??

@krzychu124
Copy link
Member Author

Also, I think we could change that if..else for missing translation keys to cover Stable and TEST till we add missing options (as translation keys are not final yet).

??

You've created some code in LookupTable to show stripped translation key if not found. Instead of "¶Checkbox: Something something" it shows "Something something"
It does not work on TEST build and few users already reported that, unless you changed something since implementation: 3960c45

@krzychu124 krzychu124 added the Dependent This issue is blocked by another issue. label Mar 2, 2022
@krzychu124
Copy link
Member Author

Waiting for #1444

@originalfoo
Copy link
Member

originalfoo commented Mar 4, 2022

Just been testing this again and it still feels a bit weird the association of Disable Despawning and Allow Despawn.

I noticed also that the Disable Despawning mod option was at some point in the past inverted from Allow Despawn meaning we have weirdness like this to contend with in (de)serialization:

// OptionsManager.LoadData():
GameplayTab_VehicleBehaviourGroup.DisableDespawning.Value = !LoadBool(data, idx: 15, true); // inverted

// OptionsManager.SaveData():
save[15] = (byte)(Options.disableDespawning ? 0 : 1); // inverted

Would it make sense to rename the Disable Despawning option to Allow Despawning (and then the Allow Despawn filters seem a bit more natural in that context)? It would mean we could remove the weird byte inversion too, so existing saves would work as expected.

Also it would mean that Options.AllowDespawn.Value == AllowedDespawnVehicleTypes > 0 at any given time, making interaction/sync between filters and checkbox much more semantic.

@krzychu124
Copy link
Member Author

To me making an option: Allow despawn suggest that despawning is disabled by default.
I don't like messing with options since the model is pretty fragile. Up to you, just don't break anything 😂

@kianzarrin
Copy link
Collaborator

image

I think multi-select dropdown is more suitable here (I have used it a lot if you needed any help).

Copy link
Collaborator

@kianzarrin kianzarrin left a comment

Choose a reason for hiding this comment

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

see comments above

@krzychu124
Copy link
Member Author

I think multi-select dropdown is more suitable here (I have used it a lot if you needed any help).

Keep in mind that modal opens when you right click despawning button on the main menu in game (tooltip needs an update). I wanted to create single solution for all.

@krzychu124
Copy link
Member Author

I've added ExtVehicleType to IsFlagSet and callback to make the button read-only when Disable despawning is not active

Copy link
Collaborator

@kianzarrin kianzarrin left a comment

Choose a reason for hiding this comment

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

Cool.

Maybe it would be nice to indent that button if possible to make it clear it belongs to Disable de-spawning. (I have some code for that in my mod if you needed help)

@krzychu124
Copy link
Member Author

@kianzarrin I'm not sure if necessary, I already tweaked the button text so IMO it's pretty descriptive what it does and when it is active:

image
image

@originalfoo
Copy link
Member

@kianzarrin can you post link to your code for indenting buttons and I'll add that to the base button class in separate PR as part of 11.6.5.2 branch.

@krzychu124 krzychu124 removed the Dependent This issue is blocked by another issue. label Mar 6, 2022
Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kianzarrin
Copy link
Collaborator

@aubergine10 my code actually puts the checkbox and the button in the same raw
https://github.com/kianzarrin/LoadOrder/blob/260f79ba8287040ab8c78e0c29d02e76370977d9/LoadOrderMod/Settings/Settings.cs#L39

but it can be modified to indent the button. we need to add a panel. I think this should work:

            var container = button.parent as UIComponent; // not sure about this line. 
            var panel = container.AddUIComponent<UIPanel>();
            panel.width = container.width;
            panel.height = button.height;

            button.AlignTo(panel, UIAlignAnchor.TopLeft);
            button.relativePosition += new Vector3(10, 0);

@krzychu124 krzychu124 merged commit ce17b6a into master Mar 6, 2022
@krzychu124 krzychu124 deleted the feature/allow-despawn-by-type branch March 6, 2022 22:42
@originalfoo originalfoo mentioned this pull request May 9, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DESPAWN TOOLS Feature: Clear traffic, toggle despawn, vehicle/parked/cim despawns feature A new distinct feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable despawning by vehicle type
3 participants