prochitecture / blosm

GNU General Public License v3.0
11 stars 3 forks source link

item.street was not updated after the street extension #105

Closed vvoovv closed 2 months ago

vvoovv commented 3 months ago

The problem still persists.

street.id == 2
# Section
street.head.street.id == 2
# MinorIntersection
street.head.succ.street.id == 2
# Section
street.head.succ.succ.street.id == 2
# MinorIntersection
street.head.succ.succ.succ.street.id == 8
# Section
street.head.succ.succ.succ.succ.street.id == 8
polarkernel commented 3 months ago

I know. I discovered a hidden modification of an iterable during iteration (exactly the same case as in issue #85). I need to rewrite this function. I'll let you know when it's fixed.

polarkernel commented 3 months ago

The problem is more complicated than I first thought, my solution was too straightforward. When a minor intersection is processed, it becomes an instance of MinorIntersection in the linked list of a Street. Its node in the way network must be removed. The minor streets connected to this intersection can no longer be hold as edges of the way network, because one of their nodes is inside the new street. It is even possible that both of their ends are minor intersections, so they are no longer held by the network at all.

I do not yet have a solution, but I assume that a different container than manager.waymap has to be used to present the instances of Street to the Blender addon.

vvoovv commented 3 months ago

Can the data structures for rendering be created in 2 passes?

In the first pass, the existing data structures (without MinorIntersection) are created.

In the second pass, Intersections are replaced with MinorIntersections and Streets are extended.

polarkernel commented 3 months ago

Can the data structures for rendering be created in 2 passes?

I assume yes.

In the first pass, the existing data structures (without MinorIntersection) are created.

The results of the first pass would be passed by manager.intersections, and manager.waymap.

In the second pass, Intersections are replaced with MinorIntersections and Streets are extended.

This should be possible. What would you expect as a result? The extended streets would include the minor intersections in their linked lists. The minor streets, that ended at these minor intersections, would still be stored in manager.waymap. But their references succ and pred would point to the connectors, that already existed in their normal intersections. But these connectors are moved to and held by the minor intersections and no longer by the normal intersections. So manager.waymap would deliver them, but already with the new connectors.

The major streets in manager.waymap would no longer be valid, unless all their contents were completely copied to the extended streets.

vvoovv commented 3 months ago

Another idea.

Currently the method waymap.iterSections() iterates through Streets. I suggest using only Sections and Intersections in the WayMap. So the method waymap.iterSections() interates through Sections. A new method waymap.iterStreets() would iterate through Streets by calling the method waymap.iterSections() to iterate through Sections and constructing Streets.

In particular, the method waymap.iterStreets() marks all Sections in the current Street as visited, so they won't be used twice. Also the method transforms an Intersection to a minor Intersection (if it is a minor one) by setting the attributes toLeft and toRight.

polarkernel commented 3 months ago

Another idea.

Thanks for this proposal. In my opinion, not the streets are a problem, but the intersection nodes in waymap. For side-lanes and sym-lanes, it was easy: Insert them and their adjacent streets into a Street, remove their intersection nodes and the adjacent streets from waymap and insert the combined Street into waymap. But MinorIntersections are different. They have to be inserted in Street together with their adjacent streets, but must remain somehow in waymap, to hold the minor streets. This is also the case for your proposition, because the minor streets remain.

Would you also be happy to get a list (or possibly a dict) as Street container (manager.streets for example), instead of waymap? Then I could find first all minor intersection nodes, then extract all streets into this container. Then the long streets with minor intersections could be merged, their streets removed from this container, and the long streets be inserted. The intersections, that have become minor intersections, would finally be removed from manager.intersections.

I don't know if you would require also a container of minor intersections, similar to manager.transitionSideLanes and manager.transitionSymLanes.

Finding all minor intersection nodes first seems also resolve the "modify iterable while iteration" problem. I think I am close to a solution.

vvoovv commented 3 months ago

In my opinion, not the streets are a problem, but the intersection nodes in waymap.

I suggest not changing the WayMap at all. Intersections can have an attribute minor. Intersections with the attribute minor==True can have a method "makeMinor()" which sets the attributes toLeft and toRight. So those minor Intersections expose the attributes of both major and minor Intersections.

Would you also be happy to get a list (or possibly a dict) as Street container (manager.streets for example), instead of waymap?

Yes. But the solution with making Streets in the iterator iterStreets() has an advantage that memory is not used for storing Streets.

I don't know if you would require also a container of minor intersections, similar to manager.transitionSideLanes and manager.transitionSymLanes.

I think instead of the list manager.intersections, 2 lists manager.majorIntersections and manager.minorIntersections will be needed.

polarkernel commented 3 months ago

Just to clarify the current state: waymap edges are always Streets. Most of these streets hold only one Section. Only few of them have an additional SymLane or a SideLane. The function name iterSections() is from the time when Sections were stored in waymap and I should actually rename it to iterStreets(), to be consistent. There are some other methods that also use Section instead of Street.

I suggest not changing the WayMap at all. Intersections can have an attribute minor. Intersections with the attribute minor==True can have a method "makeMinor()" which sets the attributes toLeft and toRight. So those minor Intersections expose the attributes of both major and minor Intersections.

waymap nodes hold an object, for example, an Intersection. I will try to replace it with an instance of MinorIntersection and see, if it complains about being modified, and I might run into the "modify iterable while iteration" problem again. If not, this might be a solution.

But the solution with making Streets in the iterator iterStreets() has an advantage that memory is not used for storing Streets.

Following my remarks above, iterStreets() could do this. Currently, the method returns the objects stored in all edges in waymap. To implement your idea, the long street has to be extracted from the network somehow. The problem is that you do not know where to start a street. It could happen that you hit a MinorIntersection so that you have to search in both directions from there. However, I will think about this idea, it could have some advantages.

vvoovv commented 3 months ago

I will try to replace it with an instance of MinorIntersection and see, if it complains about being modified

I suggested having a single class Intersection for both major and minor intersections. It is not possible to modify a dictionary or a list during an iteration through it. However if an object is a member of a dictionary or a list, then it's possible to modify the object during the iteration, e.g. set its attribute or call its method.

It could happen that you hit a MinorIntersection so that you have to search in both directions from there.

Yes, the search is need in both directions to compose a Street.

polarkernel commented 3 months ago

I suggested having a single class Intersection for both major and minor intersections.

OK, I will rewrite StreetGenerator using this idea and realize the extraction of (long) Streets from waymap. This will take a moment.

polarkernel commented 3 months ago

I am close to a solution, but there is one problem I need your help to solve:

The extended Streets are created in iterStreets(). There they need to be styled by styleStore and setStyle. I think, passing them as arguments of iterStreets() is complicated. A better way would be to provide them as attributes of the class WayMap. But manager.waymap is already constructed in manager.py. There, styleStore and setStyle do not seem to be available. Do you know an easy way to get them there, or do you have another idea?

vvoovv commented 3 months ago

styleStore and getStyle are now available in the constructor of WayManager where an instance of WayMap is created.

vvoovv commented 3 months ago

I think iterStreets should be in WayManager, since WayMap has nothing to do with street styling.

polarkernel commented 3 months ago

styleStore and getStyle are now available in the constructor of WayManager where an instance of WayMap is created.

Thanks.

I think iterStreets should be in WayManager, since WayMap has nothing to do with street styling.

OK, I can put it there, as a method of WayManager. But the extracted streets need to already have a style, I think.

There is a general issue with the idea of constructing long streets with minor intersections on the fly in iterStreets(), that I hadn't thought about before. To construct parallel streets, a prerequisite of Bundles, it is necessary to extract all streets from waymap. So this code may be executed twice.

vvoovv commented 3 months ago

OK, I can put it there, as a method of WayManager. But the extracted streets need to already have a style, I think.

styleStore and getStyle (needed for styling) are available as attributes of WayManager.

So this code may be executed twice.

Can it be optimized somehow?

polarkernel commented 3 months ago

Can it be optimized somehow?

The problem is even worse, I forgot this. Finding parallel streets requires random access to the streets. They (their indexes) are first inserted into a spatial index. Then neighbors can be quickly found and be compared by retrieving them using their index. There is no way around storing the streets, I think.

vvoovv commented 3 months ago

If so, Streets should be constructed elsewhere rather than in the method iterStreets.

polarkernel commented 3 months ago

If so, Streets should be constructed elsewhere rather than in the method iterStreets.

Building them with iterStreets was not a bad idea. The waymap gives a good guide to follow the individual streets through the minor intersections, which makes this process quite efficient. But then, they need to be stored in a container, so that they can be used multiple times, without having to be recalculated. I am not yet sure whether a Python list or a dictionary will be a suitable container. I propose to implement an iterator in WayManager, that will return all the roads to the addon. If I need to change the road container, I can easily adapt this iterator.

vvoovv commented 3 months ago

I propose to implement an iterator in WayManager, that will return all the roads to the addon. If I need to change the road container, I can easily adapt this iterator.

Ok.

polarkernel commented 3 months ago

OK, I committed a new version. The changes are:

vvoovv commented 3 months ago

After the latest commit instances of the SideLane class doesn't have the attribute styleBlock.

polarkernel commented 3 months ago

After the latest commit instances of the SideLane class doesn't have the attribute styleBlock.

Did it exist before? In the old code, the style of a street including a SideLane has been set by

            streetStyle = self.styleStore.get( self.getStyle(street) )
            street.style = streetStyle
            street.setStyleBlockFromTop(streetStyle)

Now, for every street provided by iterStreetsFromWaymap() I added

            streetStyle = self.styleStore.get( self.getStyle(longStreet) )
            longStreet.style = streetStyle
            longStreet.setStyleBlockFromTop(streetStyle)

Should I call setStyleForItems()?

vvoovv commented 3 months ago

Did it exist before?

Yes, it was set in the method street.setStyleForItems() called in the method finalizeOutput().

polarkernel commented 3 months ago

Yes, it was set in the method street.setStyleForItems() called in the method finalizeOutput().

Uups, my cleaning was one step too hard. Fixed in new commit.