Modify

Opened 2 years ago

Last modified 2 years ago

#18970 new enhancement

Way.hasIncompleteNodes() should remember the result

Reported by: GerdP Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: validator mapcss performance Cc:

Description

Since Way.isUsable() calls this method we should store the result in a Boolean instead of doing the

Arrays.stream(nodes).anyMatch(Node::isIncomplete);

again and again. Most ways don't have incomplete nodes.

Attachments (2)

18970.patch (1.0 KB) - added by GerdP 2 years ago.
Would this be the right way to do it?
18970.2.patch (13.3 KB) - added by GerdP 2 years ago.
use flags, remember status also for relations, add unit tests

Download all attachments as: .zip

Change History (10)

Changed 2 years ago by GerdP

Attachment: 18970.patch added

Would this be the right way to do it?

comment:1 Changed 2 years ago by Don-vip

A flag is better. No need to add an additional boolean for all ways where a single bit is needed.

comment:2 Changed 2 years ago by Don-vip

While you're at it, you can modify the Relation class to also use this new flag for hasIncompleteMembers() implementation.

comment:3 Changed 2 years ago by GerdP

OK, I try. my first patch doesn't work anyway as a way is not updated when a node changes its status regarding incompleteness. Please check OsmPrimitive.setIncomplete(). Why does it fire PrimitiveRemoved or PrimitiveAdded?
This looks like a copy&paste error made in r3348, code for setDeleted() and setIncomplete() were both changed.

comment:4 in reply to:  3 Changed 2 years ago by Don-vip

Replying to GerdP:

Please check OsmPrimitive.setIncomplete(). Why does it fire PrimitiveRemoved or PrimitiveAdded?
This looks like a copy&paste error made in r3348

Comes from r2623.

comment:5 Changed 2 years ago by GerdP

OK. I found no code which actually evaluates the boolean flag.

comment:6 Changed 2 years ago by GerdP

I wonder if this could explain some of the "IAE: Node is already deleted" errors?

Changed 2 years ago by GerdP

Attachment: 18970.2.patch added

use flags, remember status also for relations, add unit tests

comment:7 Changed 2 years ago by GerdP

Please review. I don't fully understand the meaning of the allowWithoutDataset flag in OsmPrimitive.referrers(boolean allowWithoutDataset, Class<T> filter)

comment:8 Changed 2 years ago by simon04

Is the performance of Way.hasIncompleteNodes an actual issue? I dislike the complexity of the update mechanism – it's spread over many functions and hard to reason about…

An alternative approach could be to cache ALL_COMPLETE_NODES as flag (this assumes that a node will never transition from complete to incomplete), such as shown in this draft:

// org.openstreetmap.josm.data.osm.Way
    public boolean hasIncompleteNodes() {
        if ((flags & ALL_COMPLETE_NODES) != 0) {
            return false;
        }
        boolean isIncomplete = Arrays.stream(nodes).anyMatch(Node::isIncomplete);
        if (!isIncomplete) {
            updateFlags(ALL_COMPLETE_NODES, true);
        }
        return isIncomplete;
    }

    private static final short ALL_COMPLETE_NODES = 1 << 14;

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to GerdP
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.