-
Notifications
You must be signed in to change notification settings - Fork 303
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
base: master
Are you sure you want to change the base?
Conversation
c131f60
to
9d9b3c8
Compare
164fabe
to
d8855a9
Compare
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 :) |
d8855a9
to
5dad0cb
Compare
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
5dad0cb
to
55a01e7
Compare
There was a problem hiding this 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 :)
public segments: number; | ||
public tileCenter: THREE.Vector3; | ||
|
||
private _refCount: { |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
7e84c08
to
865316d
Compare
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
BuilderEllipsoidTile
being changed toGlobalTileBuilder
to match the naming of thePlanarTileBuilder
(especially considering its position inside aGlobe
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.computeBuffers.js
intocomputeBuffers.ts
, changing algorithm to take advantage of simple math to avoid using slow JS constructs in favor ofArrayBuffer
s 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)