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

managers should provide reset functions for traffic rules. #623

Closed
kianzarrin opened this issue Jan 25, 2020 · 9 comments · Fixed by #639
Closed

managers should provide reset functions for traffic rules. #623

kianzarrin opened this issue Jan 25, 2020 · 9 comments · Fixed by #639
Assignees
Labels
feature A new distinct feature
Milestone

Comments

@kianzarrin
Copy link
Collaborator

kianzarrin commented Jan 25, 2020

Introduction

For each segment end flag like entering blocked junction allowed as an example (why did I chose the longest name?!) we can have these states:

States:
- NotConfigurable(NA) (default 1 or default 0)
- Not Set: true by default or false by default.
- Forced: true or false

when we set a flag we force its value. if we force set it to default value it will not change if default changes (eg. change options or change road configuration).
if we don't set the state then default value is used. Not configurable means that the default value cannot be changed.

we have also these functions for every flag:

ToggleEnteringBlockedJunctionAllowed
SetEnteringBlockedJunctionAllowed bool
- NA => force set to input value 0 or 1(the values are lost on game load).
- else => force set to input value 0 or 1 (preserved on game load)

IsEnteringBlockedJunctionAllowed bool 
- not set => return default
- forced => return forced
- NA => return default

GetDefaultEnteringBlockedJunctionAllowed bool  
- set => return default
- not set => return default
- NA => return default

IsEnteringBlockedJunctionAllowedConfigurable bool  TICK logic
- NA => return true
- else => return false

GetEnteringBlockedJunctionAllowed => TernaryBool
- Not set => return TernaryBool.undefined
- NA  => if not set then return  TernaryBool.undefined else returned forced (true or false)
- forced => forced (true or false)

Several problems:

  • (main problem) There is no interface for clearing forced values. this is important for Undo/delete feature #568 . Note that force setting the flag to default value is not same as not having the flag in the first place because the default value CAN change (change options or add/remove/upgrade segments). see screenshot.
  • SetEnteringBlockedJunctionAllowed does not check if IsEnteringBlockedJunctionAllowedConfigurable. This condition does not matter when using UI but can be reached by programming interface.
  • forced values are removed when they become un-configurable. But this does not work with the corssings mod. you can put a crossing, set the U turn and then remove crossing but the U turn remains. it is lost on next game load though.

In the picture bellow I toggle every arrow a couple of times to force set it to its default value. but when I delete a segment the lane arrows do not change.
Screenshot (328)

Solution:

  • introduce Clear<traffic rule>() methods in the interface
  • Set<Traffic rule> to check for Is<Traffic rule>Configurable . This prevents programming errors.
  • lane arrow manger to check for invalid arrow directions when segment layout changes.
@kianzarrin kianzarrin added feature A new distinct feature triage Awaiting issue categorisation labels Jan 25, 2020
@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Jan 26, 2020

Solving this issue can give a big boost to #542. Is that OK if I start working on this now? if so please assign it to me (so that I know you agree with my approach)
tagging @aubergine10 @kvakvs @krzychu124

Also I think it would be better to overload Set( ... bool value) by Set( ... TernaryBool value) to be able to clear traffic rule.

EDIT: this issue should be fixed in multiple stages. for the first stage I will provide Set( ... TernaryBool value) and in the junction restriction tool I provide del hotkey. I will do the same to lane arrows in a different pull request.

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Feb 1, 2020

Almost done here EDIT: with clearing junction restriction flags:

  • Set() functions were overloaded to accept TrinaryBool input.
  • They check for IsConfigurable (return false if not).
  • They silently return success if input value is the same as flag value.(this means they call OnSegmentChange IFF flag value actually changes).
  • I added a lot of doc to explain what I have explain in the introduction of this issue.

one problem.

Why does lane connector have configurable hotkey for delete? delete and backspace do the same thing (I have tested it). I am guessing this is a bug:
Screenshot (378)

I think all tools should use delete for removing traffic rules for all tools. no need to make it configurable. while configurability is good idea this is just too much!

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Feb 1, 2020

maybe we can assign backspace to remove traffic rules of a particular tool and delete for removing all traffic rules?

EDIT: this is dangerously confusing!

@originalfoo
Copy link
Member

Just random thought on this:

- NotConfigurable(NA) (default 1 or default 0)
- Not Set: true by default or false by default.
- Forced: true or false

Might be better as:

  • Vanilla - just use vanilla value
    • I realise this is conceptually different to 'immutable'; maybe we need (separate?) immutable state?
  • Default - TM:PE applied default (eg. user defined default road speed, or set default policies, etc)
    • Fallback to Vanilla value
  • Manual - user made customisation at node/segment/lane-level
    • Fallback to Default value

@originalfoo
Copy link
Member

maybe we can assign backspace to remove traffic rules of a particular tool and delete for removing all traffic rules?

  • Backspace -- for selected thing(s), remove customisations made by active tool
  • Alt+Backspace -- for selected thing(s), remove customiations made by all tools

@kvakvs
Copy link
Collaborator

kvakvs commented Feb 2, 2020

On Mac keyboards Delete does not exist as a separate button, you have to press Fn + Backspace, hence Backspace was added for convenience. Don't give them different meaning, for some users its the same Backspace key.

kianzarrin added a commit that referenced this issue Feb 10, 2020
#623 I provided Interface for resting junction values. to do so provide ternarybool.undefined to a set<traffic rule>() function
provided documentation explaining what I explained in description of #623.
User Interface: choose Junction restriction tool -> select a junction -> press delete
added KeyDown keybind settings mimicking Input.KeyDown()
@originalfoo
Copy link
Member

@kianzarrin Does this issue need retaining - eg. for speed limits and parking restrictions there is still no way to reset customisations.

@kianzarrin
Copy link
Collaborator Author

@aubergine10 for speed limits and parking restrictions there is still no way to reset customisations.

I don't know about them. Isn't parking restriction like priority sings, the states are:

  • Not applicable
  • allowed
  • not allowed

so to restore to defaults I should just set it to allowed (if applicable).

I thought we have a default speed limit. If my recollections are correct setting the default speed limit is same as not setting at all with the exception of the overlay symbol that is shown to the user and storage space. not ideal but its not a big issue.

I was under the impression that we have a 1+ issue per Pull request policy. if that is the case create new issues for them if not reopen this one. It has to be you because I don't use those tools too much.

@originalfoo
Copy link
Member

Continued in #692

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new distinct feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants