Re: [Potlatch-dev] Introducing Magic Roundabout tool
On Sat, Feb 26, 2011 at 2:52 AM, Dave Stubbs wrote:
> Yeah, what we have is a nested and OO implementation of a simple stack
> with markers. This allows you to do cool stuff like encapsulate
> complex actions without bothering the user with extra undo steps or
> adding lots of boiler plate code around the "RecordAction" stage.
Don't get me wrong, I think the implementation is fantastic. The only
thing I'm commenting on is some of the unintuitive naming and
mechanics.
For example, passing a function to an operation, where the function
represents a storage operation for an undoable action...is not very
intuitive. In practice, exactly one of two things gets passed to those
functions: either MainUndoStack.getInstance().push, or some
CompositeUndoableAction.addAction. It would therefore be possible to
replace declarations like this:
public function createWay(tags:Object, nodes:Array,
performCreate:Function):Way {
var way:Way = new Way(nextNegative, 0, tags, true, nodes.concat());
performCreate(new CreateEntityAction(way, setWay));
...
with
public function createWay(tags:Object, nodes:Array, actionStack:
CompositeUndoableAction=null):Way {
var way:Way = new Way(nextNegative, 0, tags, true, nodes.concat());
recordAction(actionStack, new CreateEntityAction(way, setWay));
...
where recordAction is:
protected function recordAction(stack: CompositeUndoAbleAction,
action: Action):void {
if (stack)
stack.push(action)
else
MainUndoStack.getInstance().push(action);
}
Personally, I think passing either a composite action stack, or null,
to an operation is a lot more intuitive than passing a function. And
it would make some simpler:
w = createWay({}, nodes);
Now, the fact that it's creating an action which is being placed on
the main undo stack is totally invisible. Magic behind the scenes, and
all that.
Steve
___
Potlatch-dev mailing list
Potlatch-dev@openstreetmap.org
http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] Introducing Magic Roundabout tool
On Sat, Feb 26, 2011 at 12:43 AM, Andy Allan wrote: > This is where I could be wrong, but I think that it's critical to the > redo part. When undo'ing an action the action is added to the redo > list, so if adding an action has side-effects that'll blow up during > redoing the action. Also, if the action works based on side effects > that aren't tracked within the action itself, then it won't work > properly when being redone. Ok, I obviously wasn't explaining myself well, so I'll let some code do the talking: http://trac.openstreetmap.org/changeset/25431 This solves my problem: actions are carried out immediately, and then added to the MainUndoStack later. It's not that the actions had "side effects", I just needed access to the "main effect" immediately. The difference is from this: Create action A Create action B Create action C Add to main undo stack - do action A - do action B - do action C To this: Create action A - do action A Create action B - do action B Create action C - do cation C Add to main undo stack - (already done). I might be overlooking something, of course. I still get a couple of spurious entries in the undo list which I haven't really investigated yet. And there were some weird fake duplicate nodes being shown, which might be a symptom of node state not being managed properly. Steve ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] Introducing Magic Roundabout tool
On Sat, Feb 26, 2011 at 12:47 AM, Andy Allan wrote: > Without fully understanding things - do you? Instead of splitting the Well, I'm sure that it's theoretically possible to write virtually any code in such a way that I don't need the intermediate state. But it might be less readable. And it's harder. And probably the Actions need to be clearer about their postconditions. For example, it should be possible to know for certain where the new way generated by SplitWayAction will figure in the node's parentWays. (It's probably deterministic, but it's not documented.) Without having access to intermediate state, code is probably also going to be more fragile, because you can't do sanity checks. > way and deleting one half[1] are you not instead figuring out which > nodes to delete from the way? If you look at my commit, I actually wrote two completely different versions - because I couldn't figure out for the life of me why the first one (the one that's not commented out) didn't work. > Given that you know before you start > which end of the ways you'll delete from, then it's probably > possible[2] to do it that way. That works for the "delete all nodes between the junction and the centre of the roundabout" approach. It doesn't work for the "delete the way that is between the junction and the centre of the roundabout" approach, because I don't know which way that is without looking through junction.parentWays. (Could be missing something obvious here.) Steve ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] Introducing Magic Roundabout tool
On Fri, Feb 25, 2011 at 1:43 PM, Andy Allan wrote: > On Fri, Feb 25, 2011 at 1:17 PM, Steve Bennett wrote: > >> Does this absolutely have to be the case? I can't quite understand, >> from a theoretical point of view, why this principle is necessary. Why >> not add the action to a stack, and also carry it out now? What's the >> benefit of maintaining the current state as long as possible, then >> doing all the actions in one flurry? > > This is where I could be wrong, but I think that it's critical to the > redo part. When undo'ing an action the action is added to the redo > list, so if adding an action has side-effects that'll blow up during > redoing the action. Also, if the action works based on side effects > that aren't tracked within the action itself, then it won't work > properly when being redone. Hence everything needs to be done by > adding things to an action, and it needs to be completely > self-contained so it can be moved back and forward between the > different stacks. > > But I might be wrong. You're not. That's exactly why you need to do it that way -- if you're doing a simple composite undoable action anyway. It may get a little confusing, but you can of course create your own UndoableAction class. In that case during the doAction method you're free to make any direct changes you like, calling other action's doAction methods to do so. The only catch is that you have to be able to put things back again how you found them (probably by calling undoAction in the right order). And then properly handle a redo. The easiest method of doing that is override CompositeUndoableAction, pushing actions as you do them, allowing the undo be handled normally, except that it'd have to clear the action list when done. It generally cleaner and safer to use the existing actions and compose. If you don't understand what I just said then I suggest sticking to the existing actions as there's less scope for breaking things! > >> And just to add a bit more incoherent rambling, would a simpler model >> be, instead of passing around these stacks everywhere, to have just a >> main undo stack with two functions: >> 1) RecordAction which adds an action to the undo stack, and carries it out. >> 2) FinishUndoBlock, which groups all actions since the last time it >> was called, into one block, which will all be undone with a single >> keystroke. Sort of the equivalent of closing a changeset. > > That's just an unclear representation of a composite action! The > RecordAction is undo.push, the FinishUndoBlock is > MainUndoStackaddAction(), and the > carrying-out-before-we're-finished is the bad idea that we need to > avoid. > Yeah, what we have is a nested and OO implementation of a simple stack with markers. This allows you to do cool stuff like encapsulate complex actions without bothering the user with extra undo steps or adding lots of boiler plate code around the "RecordAction" stage. Dave ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] Introducing Magic Roundabout tool
On Fri, Feb 25, 2011 at 1:17 PM, Steve Bennett wrote: > I > actually needed the split to have taken place so I could identify > which half of the split was the one that I wanted to delete. Without fully understanding things - do you? Instead of splitting the way and deleting one half[1] are you not instead figuring out which nodes to delete from the way? Given that you know before you start which end of the ways you'll delete from, then it's probably possible[2] to do it that way. Cheers, Andy [1] My split-way-and-get-relations-right code is sobbing at the injustice of it all [2] This is not a money-back guarantee offer ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] Introducing Magic Roundabout tool
On Fri, Feb 25, 2011 at 1:17 PM, Steve Bennett wrote: > Does this absolutely have to be the case? I can't quite understand, > from a theoretical point of view, why this principle is necessary. Why > not add the action to a stack, and also carry it out now? What's the > benefit of maintaining the current state as long as possible, then > doing all the actions in one flurry? This is where I could be wrong, but I think that it's critical to the redo part. When undo'ing an action the action is added to the redo list, so if adding an action has side-effects that'll blow up during redoing the action. Also, if the action works based on side effects that aren't tracked within the action itself, then it won't work properly when being redone. Hence everything needs to be done by adding things to an action, and it needs to be completely self-contained so it can be moved back and forward between the different stacks. But I might be wrong. > And just to add a bit more incoherent rambling, would a simpler model > be, instead of passing around these stacks everywhere, to have just a > main undo stack with two functions: > 1) RecordAction which adds an action to the undo stack, and carries it out. > 2) FinishUndoBlock, which groups all actions since the last time it > was called, into one block, which will all be undone with a single > keystroke. Sort of the equivalent of closing a changeset. That's just an unclear representation of a composite action! The RecordAction is undo.push, the FinishUndoBlock is MainUndoStackaddAction(), and the carrying-out-before-we're-finished is the bad idea that we need to avoid. > But, since I clearly don't understand how this stuff works, these > impertinent suggestions may be completely off the mark. Since I barely understand how it all works, but it certainly does work, I tend to just try to figure it out! When (if) I ever get to the point where I fully understand it then I might change my mind, but I'm not there yet. Cheers, Andy ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] Introducing Magic Roundabout tool
On Fri, Feb 25, 2011 at 11:21 PM, Andy Allan wrote: > add a separate undo step for every junction. Since only one keypress > causes MakeJunction, it should be undoable in one button press. > MakeJunction should have its own composite action and push things onto > it when it loops round like this, then passes just one action back to > the caller. Err, yes, that was certainly the goal :) > Oh, and finally, I suspect some of these composite actions should be > in net/systemeD/connection/actions - especially MakeJunction since > you've now shown with MagicRoundabout that it's a useful too for other > actions to build on top of. Good idea - didn't occur to me. > Right, well, if this all turns out to be gobbldy-gook don't worry too > much - I'm happy to rewrite them when I get a chance. If you want to > give it a go yourself I'm 100% happy to explain further or answer > questions. Thanks, I'll have a bit of a read up and explore. The big problem I was encountering was that I needed the intermediate results of some of those actions, in order to take later steps. It was no good saying "after this method, we'll create a circle, then we'll split some things, then we'll delete some ways. Ok, now go and do that stuff". I actually needed the split to have taken place so I could identify which half of the split was the one that I wanted to delete. I was since then thinking that what I might need was a kind of CompositeUndoableAction that also carries out the actions straight away (like the main undo stack), as well as piling them up for later. As your doc points out: >A Composite action doesn't actually make any changes itself - it just figures >out what list of changes are going to be needed to meet the overall goal. and >Although usually irrelevant, this becomes important when iterating over >things, and sometimes tricks like iterating backwards over the list become >necessary. You can see the ultimate in juggling while working out relations >handling during SplitWayAction since you need to insert the new way perhaps >multiple times into a given relation, but you need to work out all the insert >indexes up-front before any actual inserting is done. Does this absolutely have to be the case? I can't quite understand, from a theoretical point of view, why this principle is necessary. Why not add the action to a stack, and also carry it out now? What's the benefit of maintaining the current state as long as possible, then doing all the actions in one flurry? Also, while I'm irreverently questioning everything: >There's two conventions that don't help much in understanding, but they are >widely used This is very true. Could we perhaps change them? Maybe rename "undo" as "actionStack", and rename the ubiquitous "performAction" argument as "actionStackFunc" or something. Until last night, I honestly had no idea that actions weren't getting executed immediately. And even then I only had a suspicion about it, until I caused an infinite loop with some code that kept trying to delete a node until it ran out of nodes (which never happened because they never got deleted). So, I think these conventions are perhaps beyond quaint and have reached harmful and counterproductive. (Something about the term "undo" in particular made me assume that we were piling up a list of actions in case we needed to undo, whereas actually we're piling up a list of actions *to be done*. The fact that the user may later undo them is actually quite irrelevant.) And just to add a bit more incoherent rambling, would a simpler model be, instead of passing around these stacks everywhere, to have just a main undo stack with two functions: 1) RecordAction which adds an action to the undo stack, and carries it out. 2) FinishUndoBlock, which groups all actions since the last time it was called, into one block, which will all be undone with a single keystroke. Sort of the equivalent of closing a changeset. But, since I clearly don't understand how this stuff works, these impertinent suggestions may be completely off the mark. Steve ___ Potlatch-dev mailing list Potlatch-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/potlatch-dev
Re: [Potlatch-dev] Introducing Magic Roundabout tool
On Thu, Feb 24, 2011 at 10:47 PM, Steve Bennett wrote:
> Whoops, sorry about that. All fixed now.
Cheers.
> I've discovered some slight flakiness, in that occasionally it
> produces a bad result, and if you try to undo, you get an exception.
> I'll need to understand a bit more how to use the undo system.
Regarding the Undo system - while we all hail randomjunk as the only
one who Truly Understands how it works, I've been toiling away as an
apprentice for a while now[1] and pretty much have got to grips with
it. I've taken a few hours this morning to put together a wiki page
with some documentation, that as it says at the top, may or may not
help.
http://wiki.openstreetmap.org/wiki/Potlatch_2/Developer_Documentation/undo
I'd suggest reading that - if by magic it's comprehensible, the next
bit of this email might make more sense.
As for your code, the two^Wthree^Wfour things I can spot straight away are:
1) In DrawWay.as.magicRoundabout() you're adding two different things
to the undo stack - the first and last lines are adding a composite
action called "undo" that doesn't actually do anything. The middle
line asks MagicRoundabout to return its resulting action to the
MainUndoStack. So two ways of achieving similar goals would be:
new MagicRoundabout(currentNode(), elastic.length,
MainUndoStack.getGlobalStack().addAction);
OR
var undo:CompositeUndoableAction = new CompositeUndoableAction("Magic
roundabout");
new MagicRoundabout(currentNode(), elastic.length, undo.push);
MainUndoStack.getGlobalStack().addAction(undo);
... but since the second one would be creating a composite action, and
pushing only one action onto its list, it's a bit redundant.
2) MagicRoundabout.as#30 calls performAction multiple times - instead
you could create a compositeaction, add the new way and all the
splitways to it, and send it back in one go.
3) Assuming you do 2), then when magicroundabout finishes adding
SplitWayActions it'll need to add a MakeJunctions action, to the list
too. But MakeJunctions accesses the undostack directly (line 40) so it
breaks the undo chain. MakeJunction should be rewritten to take a
performAction function and instead of doing things itself, pass the
actions back to the caller.
4) MakeJunction calls MainUndoStack...addAction() in a loop - that'll
add a separate undo step for every junction. Since only one keypress
causes MakeJunction, it should be undoable in one button press.
MakeJunction should have its own composite action and push things onto
it when it loops round like this, then passes just one action back to
the caller.
Oh, and finally, I suspect some of these composite actions should be
in net/systemeD/connection/actions - especially MakeJunction since
you've now shown with MagicRoundabout that it's a useful too for other
actions to build on top of.
Right, well, if this all turns out to be gobbldy-gook don't worry too
much - I'm happy to rewrite them when I get a chance. If you want to
give it a go yourself I'm 100% happy to explain further or answer
questions.
Cheers,
Andy
[1] Like, years, man
___
Potlatch-dev mailing list
Potlatch-dev@openstreetmap.org
http://lists.openstreetmap.org/listinfo/potlatch-dev
