CitiesSkylinesMods / TMPE

Cities: Skylines Traffic Manager: President Edition
https://steamcommunity.com/sharedfiles/filedetails/?id=1637663252
MIT License
571 stars 85 forks source link

Undo/delete feature #568

Open kianzarrin opened 4 years ago

kianzarrin commented 4 years ago

As @kvakvs said in https://github.com/krzychu124/Cities-Skylines-Traffic-Manager-President-Edition/issues/559#issuecomment-557821726 :

The user needs to know:

  • that feature exists
  • how to enable the feature
  • what it does
  • whether it actually worked and how to revert it (redo some other way)

P.S. Change notes are not a good place where you tell about the feature :) It should be in game UI somehow, in a tooltip, help, or naturally offered via visual hints.

we need revert feature. The roundabout(#539) and priority Rd(#541) quick-edit feature activated by ctrl/ctrl+shift change priority rules, lane arrows, lane connection, and junction restrictions. Simply clearing all these rules require the use of multiple tools. undoing them is even harder (for example bringing back traffic lights.). Also if the user presses the delete button in TTL or lane connection there is no way to undo this.

I suggest to provide ctrl+z ctrl+y feature in low level base implementation.

Additionally, I suggest to expand the delete button to be available to other tools too:

related: #692

kvakvs commented 4 years ago

There is Ctrl+Z mod created by Strad (the guy who made Roundabout builder). https://steamcommunity.com/sharedfiles/filedetails/?id=1890830956 The problem is that the amount of hacks he had to do, i heard, is tremendous. Do you want to dig down that rabbit hole?

My comment was regarding each feature. Like if you do something with lane arrows, there should be a way to redo them differently right away, so that user doesn't sit stuck with something they don't like and are resorting to bulldoze.

originalfoo commented 4 years ago

I agree with @kvakvs. Take traffic lights for example. It might seem like we need an undo tool, but in reality the root issue is that the UI is cumbersome to use. If the UI was better there would be less need for undo too.

I don't think undo/redo in TMPE would be as difficult as the mod that kvakvs mentions (which provides general undo/redo for the base game), it would be more akin to the undo/redo in Move It mod - in other words we could store a stack of deltas for given tool.

For me the bigger issue is that if we add undo/redo now, it's going to impede future work on the tools. I think we need to focus on making the tools better, which might take several iterations to get it right, and only after we get the tools working really well should we consider undo/redo feature.

So, undo/redo would be a good feature, just not now. I think we should put this on pause until later date and prioritise getting the base tools working really well first.

kianzarrin commented 4 years ago

OK I understand so instead of the undo button we need to provide delete keybind in the tools to reset traffic rules to default. some tools already have it. we just need to expand.

There is still one problem though: my priority road mass edit feature changes traffic rules controlled by multiple tools. Its tedious to go through one tool to another to clear traffic rules. There are two solutions: A) #557 might solve this issue B) in the priority road just like ctrl+click adds various traffic rules, ctrl+delete removes such rules.

originalfoo commented 4 years ago

I think one thing that would definitely be useful, and already requested by multiple users (and devs), would be a "delete all customisations" feature, maybe a button in the Maintenance tab of mod options?

See: #281

originalfoo commented 4 years ago

I'd like to also consider a Click to Reset feature which could work something like so:

  1. Select a tool (junction restrictions, lane connector, whatever....)
  2. Alt+Control+Click a node or segment to reset any customisations made by the currently selected tool
    • Optionally: Shift+Alt+Control+Click = reset customisations of that tool along a stretch of road (so just like you could Shift+Click to apply speed limits or priority signs to a stretch of road, you could Shift+Alt+Control+Click to reset those customisations along the stretch of road)
kianzarrin commented 4 years ago

@aubergine10 Aubergine I'd like to also consider a Click to Reset feature which could work something like so:

1. Select a tool (junction restrictions, lane connector, whatever....)

2. `Alt+Control+Click` a node or segment to reset any customisations made by the currently selected tool

   * Optionally: `Shift+Alt+Control+Click` = reset customisations of that tool along a stretch of road (so just like you could `Shift+Click` to apply speed limits or priority signs to a stretch of road, you could `Shift+Alt+Control+Click` to reset those customisations along the stretch of road)

The reset feature is really good idea. Although it requires pressing too many keys! *Priority sign tool has a nice state machine that can undo the changes. Having a 4 stage state machine for SHIFT+CLICK and another 2 stage for CTRL+SHIFT+CLICK (quick setup) might be confusing though.

For as long as we don't have an undo button, things are going to be difficult. I suggest

  1. in every tool select the junction and then press DEL key to clear all traffic rules affected by that tool
  2. Add a clear TMPE rules tool button. click to delete all traffic rules on a junction, shift click to do it along a road/roundabout. in future we can turn this into undo.
  3. Use this https://github.com/krzychu124/Cities-Skylines-Traffic-Manager-President-Edition/issues/542#issuecomment-558339848 . the drawback is the user needs to clear traffic rules using a different tool than the one he used to set them up.
  4. A) in priority sign tool change the state machine for SHIFT+CLICK to a two stage state machine. add an option for using stop signs or yield signs (I already have added this option for CTRL+SHIFT+CLICK so we may use the same option for SHIFT+CLICK). The drawback of this approach is that the user can no longer set yield/stop signs on the highlighted road, only roads that are connected to it - not that anyone is doing this AFAIK. B) Alternatively we can assign SHIFT+SECONDARY_CLICK to remove all markings. the drawback of this approach is that in other tools right click is used to deselect (its confusing).
  5. undo work around: provide super basic undo button. I can record the segment/junction list affected by mass edit and then CTRL-Z or SHIFT-DEL can clear all traffic rules for every item on that list. this would be a very limited and simple to implement undo which can only be used once and does not exactly undo changes but rather clears them.
kianzarrin commented 4 years ago

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!

originalfoo commented 4 years ago

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:

I've no idea why it's configurable. However, Mac keyboards might be an issue:

Screenies:

image

image

image

originalfoo commented 4 years ago

Need to revisit Backspace support for #639

kianzarrin commented 4 years ago

I want to implement undo for my road selection panel. currently when user clicks the roundabout button it activates it setting the roundabout rules. clicking it again deactivates the button clearing all traffic rules related to roundabout.

For me the bigger issue is that if we add undo/redo now, it's going to impede future work on the tools. I think we need to focus on making the tools better, which might take several iterations to get it right, and only after we get the tools working really well should we consider undo/redo feature.

OK so I will create a work around for my road selection panel. It won't be a proper undo feature but it is good enough for my road selection panel. I record the state of nodes/segments of the round about before setting up the roundabout. Then I restore the state afterwards when user wants to undo (ie clicks the roundabout button again).

here is an incomplete codeL

class Record {
        class NodeRecord {
            public ushort NodeId;

            public bool TrafficLight;

            static TrafficLightManager tlMan => TrafficLightManager.Instance;

            public void Restore() {
                bool canChangeValue = tlMan.CanToggleTrafficLight(
                    NodeId,
                    TrafficLight,
                    ref NodeId.ToNode(),
                    out _);

                if (canChangeValue){
                    bool value = tlMan.HasTrafficLight(NodeId, ref NodeId.ToNode());
                    if (value != TrafficLight) {
                        tlMan.SetTrafficLight(NodeId, TrafficLight, ref NodeId.ToNode());
                    }
                }
            }

            public void Record() {
                TrafficLight = tlMan.HasTrafficLight(NodeId, ref NodeId.ToNode());
            }
        }

        public class SegmentRecord {
            public ushort SegmentId;

            public bool ParkingForward;
            public bool ParkingBackward;

            static ParkingRestrictionsManager pMan => ParkingRestrictionsManager.Instance;

            public void Record() {
                ParkingForward = pMan.IsParkingAllowed(SegmentId, NetInfo.Direction.Forward);
                ParkingBackward = pMan.IsParkingAllowed(SegmentId, NetInfo.Direction.Backward);
            }

            public void Restore() {
                if (pMan.MayHaveParkingRestriction(SegmentId)) {
                    bool parkingForward = pMan.IsParkingAllowed(SegmentId, NetInfo.Direction.Forward);
                    bool parkingBackward = pMan.IsParkingAllowed(SegmentId, NetInfo.Direction.Backward);
                    if (parkingForward != ParkingForward) {
                        pMan.SetParkingAllowed(SegmentId, NetInfo.Direction.Forward, ParkingForward);
                    }
                    if (parkingBackward != ParkingBackward) {
                        pMan.SetParkingAllowed(SegmentId, NetInfo.Direction.Forward, ParkingBackward);
                    }
                }
            }
        }

        class LaneRecord {
            public byte LaneIndex;
            public uint LaneID;

            public List<LaneRecord> OutGoingConnections;
            public List<LaneRecord> InCommingConnections;
            public LaneArrows Arrows;
            public SpeedValue SpeedLimit;
        }

        class SegmentEndRecord {
            public ushort segmentId;
            public bool startNode;

            public TernaryBool UturnAllowed;
            public TernaryBool NearTurnOnRedAllowed;
            public TernaryBool FarTurnOnRedAllowed;
            public TernaryBool StraightLaneChangingAllowed;
            public TernaryBool EnterWhenBlockedAllowed;
            public TernaryBool PedestrianCrossingAllowed;

            public PriorityType PrioirtySign;

            public void Record() {
                //TODO complete
            }
        }
originalfoo commented 4 years ago

Can this wait until 11.6 release? That kind of stuff should be centralised, for example have a memo class that remembers state for anything that TM:PE can change regarding a node, segment or lane. And we should have central undo stack, maybe also see if we can integrate with the Undo mod somehow so that user has global undo facility?

kianzarrin commented 4 years ago

Can this wait until 11.6 release? That kind of stuff should be centralised, for example have a memo class that remembers state for anything that TM:PE can change regarding a node, segment or lane. And we should have central undo stack, maybe also see if we can integrate with the Undo mod somehow so that user has global undo facility?

My intention is for user to be able to undo immediately afterwards. I do not intend to remember anything long term. so for example when user clicks CTRL+SHIFT+CLICK on a roundabout he can immediately switch back if he CTRL+SHIFT+CLICK again in a few seconds time.

My objective for now is as I said :" It won't be a proper undo feature but it is good enough for my road selection panel."

Yeah it can wait. I guess my work can be upgraded to central undo stack. or maybe we need a redesign if we want to go that route.