Id: Escape throws an error when drawing area with only one point

[quincylvania]

[Error] TypeError: undefined is not an object (evaluating 'entity.id')
    isInvalidGeometry (iD.js:60348)
    checkGeometry (iD.js:60313)
    finish (iD.js:60522)
    call (iD.js:786)
    ret (iD.js:30157)
    testBindings (iD.js:28419)
    bubble (iD.js:28477)
    (anonymous function) (iD.js:1527)

Possibly related: #5911

[maxgrossman]

hmmm... seems to be a scenario where graph.childNodesdoes not return all nodes in the way...

Is there reason why we could not just use the parent's node property? I will keep digging into childNodes function too

[jguthrie100]

I think those are changes I made some time last year to check if an area self intersects.

It probably just needs an if() statement to avoid the instance when there is only 1 node (i.e. nodes.length-2 resolves to -1 and hence entity gets set to undefined)

Just a bit busy atm, but can have a look at it later

[jguthrie100]

4860

[jguthrie100]

@maxgrossman what happens if you inspect the node immediately after graph.getChildNodes(parent) is called?

All the nodes should probably be there in that case, but in your screenshot, nodes.splice() has already been called, which will remove a node.

[jguthrie100]

@quincylvania Out of interest, graph.childNodes(way) might actually be slightly dangerous, in that modifying a variable that was set to graph.childNodes(way) will change the underlying childNodes value.

Presumeably, the underlying childNodes data should always remain constant, regardless of what the user does with any assigned variables, unless its meant to be like this?

[bhousel]

Presumeably, the underlying childNodes data should always remain constant, regardless of what the user does with any assigned variables, unless its meant to be like this?

Yes you should always _clone() the result of childNodes if you intend to modify it.

[jguthrie100]

Is it worth forcing it through in the childNodes method and _clone-ing it before returning?

[bhousel]

Is it worth forcing it through in the childNodes method and _clone-ing it before returning?

I don't understand what you're asking.

[jguthrie100]

As in:
https://github.com/openstreetmap/iD/blob/c09171d0e2c597c4d8b51accfb3b4642f64d7094/modules/core/graph.js#L107

return this._childNodes[entity.id] could be return _clone(this._childNodes[entity.id])

[bhousel]

return this._childNodes[entity.id] could be return _clone(this._childNodes[entity.id])

It's extremely hot code, so I'd rather not introduce an extra _clone when it's usually not needed.