search

njbrown / texturelab

Free, Cross-Platform, GPU-Accelerated Procedural Texture Generator
Other
735 stars 45 forks source link

Follow up to #43 #44

Closed leoschwarz closed 3 years ago

leoschwarz commented 3 years ago

I agree that moving the check there is the right thing to do, however the way it was implemented it wouldn't detect cycles introduced by connecting an in socket to an out socket. Causing #28 again. This should be addressed by this commit.

And I made sure to add the previously checked nodes to the checked array in remainsDAG. Should this function ever turn out to be a bottleneck performance wise, I guess the checked set should be removed, but at least the loop will always terminate in finite time like this (otherwise if there was a cycle to begin with it could potentially loop forever in this method, arguably it will still lead to a problem elsewhere when evaluating the nodes but at least it's one less potential source of problems).

njbrown commented 3 years ago

I agree that moving the check there is the right thing to do, however the way it was implemented it wouldn't detect cycles introduced by connecting an in socket to an out socket. Causing #28 again. This should be addressed by this commit.

Great that you caught this! thanks again!

And I made sure to add the previously checked nodes to the checked array in remainsDAG. Should this function ever turn out to be a bottleneck performance wise, I guess the checked set should be removed, but at least the loop will always terminate in finite time like this (otherwise if there was a cycle to begin with it could potentially loop forever in this method, arguably it will still lead to a problem elsewhere when evaluating the nodes but at least it's one less potential source of problems).

This won't wont be a performance bottleneck unless there are thousands of nodes in the graph, which isnt practical. Tested it on actual graph with 80 nodes and it performs very well.