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

Undo/delete feature #568

Open
kianzarrin opened this issue Nov 23, 2019 · 12 comments
Open

Undo/delete feature #568

kianzarrin opened this issue Nov 23, 2019 · 12 comments
Assignees
Labels
feature A new distinct feature MASS EDIT The mass edit tool
Milestone

Comments

@kianzarrin
Copy link
Collaborator

kianzarrin commented Nov 23, 2019

As @kvakvs said in #559 (comment) :

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:

  • lane arrow: delete => reset to default arrows
  • junction restriction => reset all rules to default
  • perhaps add delete also in speed limit and parking tools as well

related: #692

@kvakvs
Copy link
Collaborator

kvakvs commented Nov 23, 2019

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

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

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

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 originalfoo added ⏸Paused Paused for now and removed triage Awaiting issue categorisation labels Nov 26, 2019
@kianzarrin kianzarrin changed the title Unto/delete feature Undo/delete feature Nov 26, 2019
@kianzarrin kianzarrin changed the title Undo/delete feature ~~Undo~~/delete feature Nov 27, 2019
@kianzarrin kianzarrin changed the title ~~Undo~~/delete feature ~~Undo~~ /delete feature Nov 27, 2019
@kianzarrin kianzarrin changed the title ~~Undo~~ /delete feature Undo/delete feature Nov 27, 2019
@originalfoo
Copy link
Member

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

@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 Integration of Vanilla and TMPE priority roads #542 (comment) . 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
Copy link
Collaborator Author

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 originalfoo added this to the 11.1 milestone Feb 2, 2020
@originalfoo
Copy link
Member

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:

  • Most older Mac keyboards only had backspace key
  • Most newer Mac keyboards replaced backspace with delete key
  • Some Mac keyboards have both backspace and delete

Screenies:

image

image

image

@originalfoo
Copy link
Member

Need to revisit Backspace support for #639

@originalfoo originalfoo modified the milestones: 11.1, 11.2 Feb 7, 2020
@kianzarrin
Copy link
Collaborator Author

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

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

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.

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

No branches or pull requests

3 participants