derkork
commented
3 months ago I really wish you created an issue before spending time on this PR. You probably spent a lot of time on this and while I appreciate the effort I really can't merge this.
Please let me lay out my reasoning for this:
First, we have not established that there is actually a problem that needs solving. The PR is based on a hunch that something may be bad for performance without bringing any evidence. There is a performance test harness part of the project and right now 1000 state charts doing transitions require about 0.5-0.8ms of script time per frame. To me this looks absolutely reasonable and most projects will never even come close to this amount of live state charts.
Second, we don't have any evidence that this new code will actually improve things. I gave it a quick shot with the profiler and I couldn't see any difference - we were still between 0.5 to 0.8ms per frame. Now depending on how the state chart is setup and how you measure stuff numbers may be different but from what I see there is no discernible difference in performance.
Finally, this PR changes the very heart of how the library works internally. I have no doubt that it works for the examples as you tested with them, but the examples only are a smoke test and every change in transition handling can subtly change how the state chart as a whole behaves. Event order may change, evaluation order of guards may change, state activation order may change and all of this can have unexpected side effects on existing projects. As you can tell by the amount of comments I added to the logic this was very hard to get right. And your change even may have gotten it right on first try. But validating this is a lot of work for which I don't currently have the bandwidth and I would have to deal with the fallout if there were bugs introduced by this.
So all in all it just doesn't make sense to merge this. It fixes no problem, the proposed changes have no measurable impact on performance and merging it would potentially introduce new bugs.
HymnStudio
Hi, I am new to your plugin and love your work!
In the current implementation, the transition logic is eventually done by state's _handle_transition() method, where lots of recursive stuff is involved. I find that the process of determining the parent-child relationship between state nodes are performed repeatedly, things like: is_ancestor_of(target), if target in get_children(), etc. I'm worried this might have an impact on performance.
Here I try to let the state chart handle the transition, using the nodepath between source state and target state. In this way, no parent-child relationship check is needed as the nodepath tells it clearly. I tried all your example and currently found no conflicts yet.
However, as I am not familiar with godot's profiler, I can't tell if this change can really improve the performance or just do the opposite. I will be appreciated if you can review this PR or even test its performance!