Fix `VisGeometry` constructor overwriting properties it initializes

[frasercl]

Problem

This addresses a comment I left in #314: a new VisGeometry object must have its setupScene method called before it can be used, despite the fact that setupScene is already called once in its constructor.

Solution

After calling setupScene to properly initialize scene objects and place them in the scene graph, VisGeometry's constructor turned out to be reinitializing many key objects (including lights, bounding box, and the renderer itself) without the same setup. Thus the setupScene call after construction was required to recreate these objects a third time with the correct configuration. I removed initializers for objects that are initialized in setupScene from the constructor and was able to remove the setupScene call after the constructor without issue.

This creates a bit of a problem for TypeScript: it wants to be sure that all non-optional declared properties of a class are given initial values in the constructor, and is unwilling to check methods called by the constructor for initializations. There are a few possible solutions to this:

• Give all properties initialized in the method definite assignment assertions (!:). This effectively disables this check for a single property, and is what I ended up doing, though it comes at a cost of less rigorous checking.
• Give all properties initialized in the method dummy initializers before the method is called. This keeps the type system happier but adds redundant work to the code.
• Delete setupScene and inline its code in the constructor. It's not clear to me that a user of VisGeometry would ever have a use for a VisGeometry without a fully set up scene, or that anyone would want to call setupScene to reinitialize VisGeometry's most essential properties rather than creating an entirely new instance. If they definitely would not want either option, setupScene's code could just be moved into the constructor. This would make the constructor a bit bloated, but honestly not unusually so given VisGeometry's size and complexity.

[github-actions[bot]]

jest coverage report 🧪

Total coverage

Status	Category	Percentage	Covered / Total
🔴	Statements	40.16%	1951/4858
🔴	Branches	43.97%	814/1851
🔴	Functions	36.83%	400/1086
🔴	Lines	40.41%	1869/4624

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

[meganrm]

I kinda like option 3 more? I think what you wrote is right, which is this class is designed to "set up a scene" so it makes sense for that all to be part of the constructor. But I don't feel strongly about it so unless @toloudis thinks it should be done that way this seems good enough to me

[frasercl]

I kinda like option 3 more? I think what you wrote is right, which is this class is designed to "set up a scene" so it makes sense for that all to be part of the constructor. But I don't feel strongly about it so unless @toloudis thinks it should be done that way this seems good enough to me

Yeah I also basically prefer option 3, but was being cautious (probably too cautious) about making changes to the API. Given that this class is entirely internal it doesn't make much sense to think too hard about the set of use cases we should or shouldn't support; the use that is there already is likely the only one there ever will be.

[toloudis]

If setupScene is not being called anywhere else, then it's fine to inline it in the constructor. No strong preference there -- I think this code is ok but if you inlined some more of the member variable initialization in the ctor then you could get rid of those exclamation points.