New Algorithm For The Splines From Edges Node.

[OmarEmaraDev]

This new algorithm create as much continuous splines from edges as possible, resulting in more workable output for processing and rendering.

[JacquesLucke]

would prefer a str input here for isVertexRadius because "how are the radii used?" is not really a yes-no question.
There could be more than two options in theory.

[JacquesLucke]

just by convention either this should be called algorithmTypeItems or the property in the node should just be called algorithm.

[JacquesLucke]

this could raise an segmentation fault if there aren't enough vertices.
The node itself should check wether the inpu is valid (max vertex index of an edge is smaller than the number of vertices, like in the original code)

[OmarEmaraDev]

Should the node itself have error handling then?

[JacquesLucke]

yes, I think the node should do the check. "cutils" usually should not have to make expensive checks, they can expect the data to valid.
Nodes on the other hand can not expect that.

[OmarEmaraDev]

Alright.

[JacquesLucke]

in general it would be nice to use the Lists AN provides instead of doing memory allocations yourself if appropriate. The performance impact should be negilible.

[JacquesLucke]

using the append_LowLevel function here might result in a noticable performance improvement

[OmarEmaraDev]

I am wondering if it would be better to do:

splineRadii = FloatList.__new__(FloatList, length = verticesAmount, capacity = verticesAmount)
splineRadii[0] = radii.get(nonBipolarVertex)
splineRadii[1] = radii.get(nextVertex)
count = 2
for _ in range():
    ...
    splineRadii[count] = radii.get(nextVertex)
    count += 1

[JacquesLucke]

that would be even faster, but not sure if it is worth the extra variable

btw, when you specify the length of a list in the constructor, you don't need to specify the capacity (if it is the same)

[OmarEmaraDev]

We could reuse the variable j, then we won't have to create any extra variables. And it will be used in a narrow vertical scope. Should we do that?

[JacquesLucke]

reusing the same variable for completely different things is a very bad practice. Let's not do that :D

I'm not even sure why you need the extra variable if you already have variables for last/next and current vertex.

[OmarEmaraDev]

I meant to use it instead of count, I was responding to your comment:

but not sure if it is worth the extra variable.

But yah, I will no longer reuse variables, will have to edit some stuff in account with this. :D

[JacquesLucke]

maybe a better range(...) can be found so that this does not feel like a very inefficient inner loop?
Also by just looking at the way you use the loop it might be more appropriate to use a while loop instead?

[OmarEmaraDev]

I know that the number of iterations won't exceed verticesAmount and is likely to be much less, so I though a for loop with a break condition would be safer here.
What do you mean by a better range(...)?

[JacquesLucke]

just ment a better upper bound could be used in range(...).

Anyway, looks like a for loop shouldn't be used here at all. Can you try to convert it into a while loop?

[OmarEmaraDev]

Sure, that would be my first while loop 😄.

[JacquesLucke]

do you actually want to change i here? Changing it might interfere with the outer loop

[OmarEmaraDev]

I am temporarily storing the value of currentVertex in it. The variable i--- As an index of the for loop--- is not used after that line, so I though it would be OK to overwrite it.

[JacquesLucke]

the thing is this: it is not used now but you never know if it will be used in the future. By reusing it you sign up for trouble.

Also if anyone wants to convert the code to c, you can't simply reuse the variable, because you would probably end up in an endless loop or so.

[OmarEmaraDev]

I see, will make sure not to do that again.

[OmarEmaraDev]

How about this form?

while neighboursAmounts[nextVertex] == 2:
    if neighbours[neighboursStarts[nextVertex]] == currentVertex:
        currentVertex = nextVertex
        nextVertex = neighbours[neighboursStarts[nextVertex] + 1]
    else:
        currentVertex = nextVertex
        nextVertex = neighbours[neighboursStarts[nextVertex]]

currentVertex = nextVertex will be written twice, but we will avoid creating any temporary variables or extra lines of code.

[OmarEmaraDev]

I think such inline expression will also work. Which one should we use?

currentVertex, nextVertex = nextVertex, neighbours[neighboursStarts[nextVertex] + neighbours[neighboursStarts[nextVertex]] == currentVertex]

[JacquesLucke]

I still think you should use an IntegerList instead of allocating it yourself.
IntegerList.fromValue(0, length = verticesAmount)

[OmarEmaraDev]

OK, so, in general, we shouldn't use malloc if there is a list alternative.

[JacquesLucke]

Right, by using lists the code is safer, we can't forget to free and it is cleaner.

[OmarEmaraDev]

Everything now uses lists.

[JacquesLucke]

for performance reasons you should use .data when accessing list elements from cython code.

[OmarEmaraDev]

Was just going to ask you about this. Will do.

[JacquesLucke]

I'd suggest extra v1 and v2 variables of type unsigned int to reduce line length and improve readability.

[OmarEmaraDev]

Is it a good programming practice to do:

v1, v2 = edges.data[i].v1, edges.data[i].v2

or should I separate them into two lines.

[JacquesLucke]

I think in this case it is fine. You could just check if cython generates efficient code for it by checking the generated c code.
(You can create an annotated html version of the file with this command: cython -a path/to/file.pyx)

[JacquesLucke]

these lists are wayy to large in many cases. Better use the append_LowLevel function instead of allocating huge arrays for every spline.

[OmarEmaraDev]

What should the initial capacity of those lists be?

[JacquesLucke]

Just don't give them an initial capacity, or maybe 2.

[JacquesLucke]

is this used?

[OmarEmaraDev]

No, my bad.