Consider exposing fewer of the internal Tile functions to the user

[javagl]

At the time of writing this, the Tile class has >30 functions that are public.

I'm listing them here (omitting possible const versions), as an overview, and from a very short glance (just to have an estimate!), only the first half of this list should actually be public:

    Tileset* getTileset();
    TileContext* getContext();
    Tile* getParent();
    gsl::span<Tile> getChildren();
    const BoundingVolume& getBoundingVolume() const;
    const std::optional<BoundingVolume>& getViewerRequestVolume() const;
    const std::optional<BoundingVolume>& getContentBoundingVolume() const;
    double getGeometricError() const;
    TileRefine getRefine() const;
    const glm::dmat4x4& getTransform() const;
    TileContentLoadResult* getContent();
    void* getRendererResources() const;
    bool isRenderable() const;

    void prepareToDestroy();
    void setContext(TileContext* pContext);
    void setParent(Tile* pParent);
    void createChildTiles(size_t count);
    void createChildTiles(std::vector<Tile>&& children);
    void setBoundingVolume(const BoundingVolume& value);
    void setViewerRequestVolume(const std::optional<BoundingVolume>& value);
    void setGeometricError(double value);
    void setRefine(TileRefine value);
    void setTransform(const glm::dmat4x4& value);
    const TileID& getTileID() const;
    void setTileID(const TileID& id);
    void setContentBoundingVolume(const std::optional<BoundingVolume>& value);
    LoadState getState() const;
    TileSelectionState& getLastSelectionState();
    void setLastSelectionState(const TileSelectionState& newState);
    void loadContent();
    bool unloadContent();
    void update(uint32_t previousFrameNumber, uint32_t currentFrameNumber);

The second part (particularly, basically all set... functions) are only or mainly used for the initialization and setup, and maintaining the tiles structure during the rendering process. Specifically, most of them are only called by the Tileset (and few of them by other internal classes).

One could consider to reduce the size of the "surface" of this class: Users should not call things like tile->setParent(nullptr), and thus, should preferably not be able to do this.

(Some of the function may also only be relevant for certain use-cases. This should be pointed out in the documentation)

It might be possible to solve this by simply letting Tile and Tileset be friend classes, but more elaborate (or "clean") solutions may exist. (Similar questions may arise for other classes that are publicly exposed to the user, but to a much lesser extent).

[kring]

My current thinking is that we should just hide Tile from the public interface entirely. At this point I think only IPrepareRendererResources uses it, and we should be able to remove it from there pretty easily.

[javagl]

Indeed, from the current use in cesium-unreal, the main use is in prepareInMainThread, but there, only the TileContentLoadResult is required. Beyond that, it is passed around to some functions, but ... usually not used there. Should I just give it a try and see whether Tile can be removed? (If yes: We occasionally talked about a "private Impl namespace", and in some places, this is already used - should Tile also be moved to an Impl namespace for clarity while doing that?)

[kring]

Should I just give it a try and see whether Tile can be removed?

👍

If yes: We occasionally talked about a "private Impl namespace", and in some places, this is already used - should Tile also be moved to an Impl namespace for clarity while doing that?

That makes sense to me.

[javagl]

I've started that, and there are few places where the Tile is actually required in the interface - but just stumbled over one appearance where I'm not sure: When the ViewUpdateResult is processed on Unreal side, there are things like this:

for (Cesium3DTiles::Tile* pTile : result.tilesToNoLongerRenderThisFrame) {
	if (pTile->getState() != Cesium3DTiles::Tile::LoadState::Done) {
		continue;
	}

	UCesiumGltfComponent* Gltf = static_cast<UCesiumGltfComponent*>(pTile->getRendererResources());

(Edit, x2)

In general, the fact that the ViewUpdateResult exposes the tiles and requires the actual Tile objects to be present during the traversal makes this pretty difficult, even internally.

I already considered (and in fact, sketched some implementations locally) to refactor the visting and traversal, in order to achieve a state where it's possible to understand (and unit-test!) what's happening during the traversal, and this may also affect this code part. But that woulldn't be something that contributes to a quick first release, so I'm not entirely sure about the priorities or the best way forward here.

Depending on the general consensus, I could

• try to remove Tile from the interface, with all implications (maybe something like an InternalViewUpdateResult that contains the tiles, and the ViewUpdateResult (without the tiles) being returned to clients...)
• try to remove the most critical public methods from Tile (but leave the class itself as part of the public API) - maybe with some friend magic here and there...
• try to sum up my thoughts about the traversal and possible solutions for that