Graph expects intersection IDs to equal their index

[JonKo314]

Just a minor issue that needs to be considered when developing new Graph boards until/unless it's fixed: I was working on #151 again and capturing didn't really work, also I couldn't place a stone on the last intersection (lower right corner), because of an out-of-bounds error.

The problem is that createGraph() in boardFactory.ts ignores the IDs of the intersections IDs and just works with indexOf(), but the MulticolorGraphBoard uses the original IDs. And after punching a hole into the grid, I didn't change the IDs of the intersections. So I had IDs 0 to 11 and 13 to 24 instead of 0 to 23.

[benjaminpjones]

Just to check, is this affecting any existing variants? Or is mosaigo the first variant to deviate from the constraint?

Either way, I don't mind using a sparse array or Map for the underlying data type of Graph.

[JonKo314]

Just to check, is this affecting any existing variants? Or is mosaigo the first variant to deviate from the constraint?

I don't think so, but I haven't tested it. And I applied the constraint for mosaigo too, so now it works.

---

Either way, I don't mind using a sparse array or Map for the underlying data type of Graph.

Sorry, I don't understand what you mean by that. Could you elaborate please?

[merowin]

Hm should we refactor the graph board component such that it uses the graph instance instead of intersection[] ? That would ensure consistency with the variants, I think.

[benjaminpjones]

Either way, I don't mind using a sparse array or Map for the underlying data type of Graph.

Sorry, I don't understand what you mean by that. Could you elaborate please?

I believe part of the issue is that the underlying data structure for Graph expects a dense array, for example:

adjacency_list: number[][] = [[2, 3], [1, 3], [1, 2]];

however, it may help if we support sparse arrays, for example:

adjacency_list: (number | undefined)[][] = [[2, 4], [1,4], undefined, [1, 2]];

or a mapping:

adjacency_list: { [ index: number ]: number[] } = { 1: [2,4], 2: [1, 4], 4: [1, 2] }

[benjaminpjones]

Hm should we refactor the graph board component such that it uses the graph instance instead of intersection[] ?

I think this would fix "I couldn't place a stone on the last intersection (lower right corner), because of an out-of-bounds error.", but wouldn't fix "capturing didn't really work"

[merowin]

Hm should we refactor the graph board component such that it uses the graph instance instead of intersection[] ?

I think this would fix "I couldn't place a stone on the last intersection (lower right corner), because of an out-of-bounds error.", but wouldn't fix "capturing didn't really work"

I thought about this again, and actually I do believe that this would fix the problem. Here's my reasoning.

Originally the idea with the intersection IDs came from the thought that, for encoding a move on an arbitrary graph, we need a way to uniquely identify intersections. It doesn't need to be a number, e.g. it could also be the sgf representation on a grid board.

There are two places where we need to map the move to its intersection (or vise versa): 1. the variant, 2. the board component. Currently the board component uses the intersection ID as unique identifier, whereas the variant uses the intersections index in the graph class. This works for the existing boards, because we've made sure that the intersection ID equals its index in the array. However this also means that the intersection ID is superfluous, since we could just take its index in the array anyway. Also as @JonKo314 pointed out, this constraint adds complexity and a potential source for error for developers of new boards.

So my proposal is the following:

1. refactor the graph board component such that it uses the graph class instead of an array of intersections, and encode the move by its index in the graph (thus making sure that the board component and the variant uses the same mapping between intersection and move encoding).
2. remove the IDs of intersections altogether

I can do the refactoring when I have the time. Do you agree with the proposal?

[benjaminpjones]

Originally I thought ids were used to build the adjacencyList in createGraph, but you are right - this is not the case.

1) refactor the graph board component such that it uses the graph class instead of an array of intersections, and encode the move by its index in the graph (thus making sure that the board component and the variant uses the same mapping between intersection and move encoding). 2) remove the IDs of intersections altogether

I can do the refactoring when I have the time. Do you agree with the proposal?

This sounds like the right approach, thank you!