Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: migrate TileBuilder to typescript #2440

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HoloTheDrunk
Copy link
Contributor

@HoloTheDrunk HoloTheDrunk commented Oct 16, 2024

Description

Create types for the tile builder section of iTowns.
Improve performance of tile creation.
Improve code readability of tile creation.

Depends on #2447. Merged.

Summary of changes

  • Lots of renaming, most notably the BuilderEllipsoidTile being changed to GlobalTileBuilder to match the naming of the PlanarTileBuilder (especially considering its position inside a Globe folder). Hesitated between "Globe" and "Global" so feel free to criticize the choice, it's already a breaking change so we might as well pick the best one.
  • Resolve circular type dependency between builders and their parameters by making the new builder interface generic over type parameters.
  • Significantly refactor computeBuffers.js into computeBuffers.ts, changing algorithm to take advantage of simple math to avoid using slow JS constructs in favor of ArrayBuffers and moving some checks out of loops.

Motivation and Context

This fits into the larger motion towards converting iTowns to TypeScript.
Code had to be changed and moved around to allow static type checking.
Some of the functions around buffer generation were cryptic and had low readability, which required additional changes.

Tested on Ubuntu 22.04.5 LTS x86_64 using Firefox 126.0.1 (64-bit)

@HoloTheDrunk HoloTheDrunk force-pushed the tile_builder_typescript branch 3 times, most recently from c131f60 to 9d9b3c8 Compare October 22, 2024 15:23
@HoloTheDrunk HoloTheDrunk changed the title refacto: switch tile builder to typescript refactor: migrate TileBuilder to typescript Oct 23, 2024
@HoloTheDrunk HoloTheDrunk force-pushed the tile_builder_typescript branch 4 times, most recently from 164fabe to d8855a9 Compare October 29, 2024 10:06
@HoloTheDrunk
Copy link
Contributor Author

HoloTheDrunk commented Oct 29, 2024

It'd be great if I could please get a go-ahead on the general structure of this refactor so I can start working on the next one—which will inevitably be based off of this one, while the specific details of the PR get reviewed :)

@jailln jailln self-assigned this Nov 6, 2024
Improve performance by using statically-sized ArrayBuffers.
Reorganize code to get rid of some of the params/builder mess.
Cleanup computeBuffers function.

Squashed commit history (oldest to youngest):
- fix: UV_1 generation
- refacto(wip): cleanup and optimize computeBuffers
- refacto(wip): improve index generation
- wip: correct offset, off-by-one still at large
- fix: change calls to allow camera debug
- fix: found the error, off by a power of 2 actually
- fix(uv): correct indices passed to UV buffering
- fix(index): only generate buffer when needed
- wip: enable cache
- fix(computeBuffers): group tile and skirt together
- fix(wip): squash rogue private field access
- refacto: convert TileGeometry to TypeScript
- style(builders): make method visibility explicit
- refactor(exports): remove default exports
- feat(TileGeometry): add OBB type
- refactor: rename BuilderEllipsoidTile, fix imports
- fix(tileGeometry): update tests to use public api
- fix(TileBuilder): remove dead code comments
Copy link
Contributor

@jailln jailln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all these changes, it's already better and easier to understand with typings. I think we should take this opportunity to improve the architecture and the doc of this part of the code a little bit more, that's what stands out from my comments. Let me know what you think :)

src/Core/Prefab/Globe/GlobeTileBuilder.ts Outdated Show resolved Hide resolved
src/Core/Prefab/Globe/GlobeTileBuilder.ts Show resolved Hide resolved
src/Core/Prefab/Globe/GlobeTileBuilder.ts Show resolved Hide resolved
src/Core/Prefab/Globe/GlobeTileBuilder.ts Show resolved Hide resolved
src/Core/Prefab/Globe/GlobeTileBuilder.ts Show resolved Hide resolved
src/Core/Prefab/TileBuilder.ts Show resolved Hide resolved
src/Core/Prefab/TileBuilder.ts Outdated Show resolved Hide resolved
src/Core/TileGeometry.ts Show resolved Hide resolved
public segments: number;
public tileCenter: THREE.Vector3;

private _refCount: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the purpose of _refCount. From what I understand it is always initialized and increased to 1 when we create a new TileGeometry. Am I missing something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes into play with the caching used in TileBuilder.ts:newTileGeometry. You can see in convertToTile.js:convert that the call to increase the reference count is made on the result of newTileGeometry, which can be either a new TileGeometry or a TileGeometry that was retrieved from the global tile cache.
We need the _refCount to be able to dispose of the cache entry and GPU device memory only when all references to that shared TileGeometry are dropped.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the explaination, I missed the cached ones. It makes me think that newTileGeometry could be renamed to getTileGeometry, it would be less misleading.
Regarding _refCount, what do you think about leaving it as a number and putting the logic of fn in TileGeometry:dispose directly? That may be easier to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in my reply to your other comment about this topic, dispose is an overridden method from THREE.BufferGeometry so we don't have the leeway to change its parameters, which would be needed considering that the custom disposal logic makes use of references to values that ideally shouldn't be kept as additional fields within the class. There might be a better way to do this, but I currently don't see it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two issues are to tell the cache that it something must be deleted and to identify the tile to delete. In the current architecture, one way would be to add an id to TileGeometry and dispatch an event in TileGeometry.dispose trapped by the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TileGeometry already inherits from THREE.BufferGeometry, we could use a mixin to get multiple inheritance or have an EventDispatcher field but that's arguably even less readable.
In general, I'd rather avoid relying on events as much as possible: they're an easy solution in a lot of cases, but it's more of a bypass of architecture than anything and they make code flow harder to follow by creating indirection in what should otherwise be simple single-threaded logic, which is at least in part what we're trying to avoid by moving to TS. In an application this would be fine (taking for example the fact that the Godot game engine exposes a similar "signals" system and that works out fine), but for the core internals of a library I believe this is detrimental to maintainability.
Currently, despite the initial surprise of the reference counting code, its locality makes it easy to read and reason about: everything related to the handling of the reference count is in one file and easily fits into a single screen, and changes can be freely made to the cache with only minor changes required in TileGeometry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that events can hide an architectural flaw, which would indeed be the case here but I think this is also the purpose of this proposed implementation . The question is which one we'd rather have. I think that events would make it easier to read and I proposed that to avoid a too big refactoring but let's give it some thought and see what we do after.

In my opinion, the clean way to go would probably be that the object that takes care of TileGeometry disposal also takes care of removing it from the cache if the tile is not referenced anymore. By looking at the code, I think that tile cache should be stored in TileGeometryLayer instead of TileBuilder and that it should be the responsability of the TileGeometryLayer to remove obsolete tiles from its cache. What do you think ? That would remove the need to store a reference to the whole tile cache in each TileGeometry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing the tile cache in TileGeometryLayer prevents us from reusing tiles between layers, which could lead to a pretty significant increase in memory usage in complex use cases.
The cache can't be stored in TileBuilder either since it's an interface.
After discussing it with @Desplandis we came to the conclusion that the best option would be to create a static alternative constructor for TileGeometry that would return a Proxy over TileGeometry with the caching logic built-in.
As the entire tile generation pipeline is likely to change in the medium term (say, within the next year or so if we don't run into too many issues), we're waiting for that to really kick the current pipeline to the curb and build anew.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's discuss it further at that time. Meanwhile, can you add a comment above refCount to explain why it is implemented like this please? (essentially kind of a summary of what we said here, or maybe even a link to this discussion?)

src/Core/TileGeometry.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants