Prevent infinite recursion when visualizer mappings of the same node are combined together

[PathogenDavid]

This is potentially not the most elegant solution possible here, but Gonçalo asked me to take a quick look and see if we could squeeze a fix in for 2.8.3.

Maybe this is good enough to just prevent the crash since we don't expect anyone to actually intentionally make graphs like this and this code only runs on startup. If not we could leave #1769 open and do a more thorough investigation later.

I'm not attached to the wording of the message if anyone has a better suggestion. (Might be more accurate to say the same node appears multiple times in the mashup? Not sure if people would actually consider VisualizerMapping associations to be mashups though, that feels like internal nomenclature leaking to the surface.)

For some reason the error dialog shows up twice, but I think that's actually an unrelated bug. (The exception is only actually thrown once, at first I thought it was getting thrown for each VisualizerMapping node but I verified that's not the case.)

Fixes #1769

[glopesdev]

@PathogenDavid I was looking over this again and I think the infinite recursion only happens in the specific case where the same mapping is being called over itself, in which case we can detect it by just testing for mapping.Source == builder.

I believe this to be the case since we know the graph is a DAG by definition, so no real cycles. I believe the only reason we get a call cycle here must therefore be by propagation of the same builder onto itself, which can only happen under the small set of no-modification rules, and crucially, it happens only for the same builder reference. As soon as that builder is modified by some interaction with other nodes, it becomes a different builder, and therefore overlaying that on top of the original builder is completely fine.

If this turns out to be true, then it also raises the question of whether we should throw at all, or simply ignore or better handle this specific case.

The motivation to ignore would be that overlaying the exact same visualizer onto itself (with exactly the same data) is effectively a no-op, i.e. you would show the same thing no matter what. We could consider such operation to be idempotent and therefore simply ignore the recursion, e.g. in GetMashupArguments do:

            return visualizerMappings.Where(mapping => mapping.Source != builder).Select(mapping =>

The only potentially strange situation here would be the case where we map the exact same builder onto itself but using a different visualizer type (i.e. map the same image onto itself as text for visualization purposes). Under this proposal, this stacking would simply be ignored, which might be unexpectedly weird.

However, I feel in this case throwing might not be the right answer either, and ideally we would like to figure out a good way to handle correctly the mashup between the two.

[PathogenDavid]

in which case we can detect it by just testing for mapping.Source == builder.

Sounds good to me, the HashSet recursion detection was out of an abundance of caution in case there was a more complex setup we weren't seeing.

If this turns out to be true, then it also raises the question of whether we should throw at all, or simply ignore or better handle this specific case.

We definitely can't just ignore it. @RoboDoig found that with the following workflow the data mashup basically just doesn't happen (the point is gone) if you just ignore the "cycle": PointOnImage-IssueWithVisualizerMappingCycle.zip

I think we talked about this during our meeting and IIRC you thought this was partially/wholly caused by #1789, but testing it with a timer (as shown above) has the point being lost entirely if the bottom visualizer mapping is enabled. (The camera view actually updates live and the #1789 problem doesn't even happen in the first place.)

The current tip of this PR's branch has a preprocessor toggle to toggle between skipping the recursion and throwing if you want to poke at it. (I personally still need to dig in further to get a better view of the big picture here.)

---

Another reason we might want to throw for now is so we can more easily change the semantics of this currently-odd case in the future. For example, to handle the intentional self-mashup scenario we were talking about during dev club yesterday. (IE: The mapping a camera capture's info onto its image and such.)

[glopesdev]

Another reason we might want to throw for now is so we can more easily change the semantics of this currently-odd case in the future.

This is a good point, maybe in that case let's just throw with the simple test for now and we can expand the semantics for the next feature release.

[PathogenDavid]

Cleaned up the branch to be the throwing implementation. Was slightly unsure on the exception message since as far as I know we never use the "mashup" wording in documentation and such, aimed for something that hopefully makes sense to anyone unfortunate enough to encounter this without thinking anything is wrong.