HARP-15210: Wait for texture GPU upload on TileGeometryCreator. #2225
Conversation
atomicsulfate
force-pushed
the
HARP-15210
branch
3 times, most recently
from
bfaf327
to
6fc8b84
Compare
TileGeometryCreator.createAllGeometries() returns now a promise that is resolved when all textures in the tile are uploaded in GPU. Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
atomicsulfate
force-pushed
the
HARP-15210
branch
from
6fc8b84
to
8f3965f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2225 +/- ##
==========================================
+ Coverage 67.16% 67.29% +0.13%
==========================================
Files 312 312
Lines 27680 27696 +16
Branches 6183 6184 +1
==========================================
+ Hits 18590 18638 +48
+ Misses 9090 9058 -32
Continue to review full report at Codecov.
|
| } | ||
| this.finish(); | ||
| tile.dataSource.requestUpdate(); | ||
| geometryCreator.createAllGeometries(tile, decodedTile).then(() => { |
There was a problem hiding this comment.
I think it is nicer to mark the function createGeometry as async (from a documentation point of view). The added benefit is that you can await here.
| */ | ||
| createAllGeometries(tile: Tile, decodedTile: DecodedTile) { | ||
| createAllGeometries(tile: Tile, decodedTile: DecodedTile): Promise<void> { |
There was a problem hiding this comment.
You can (but don't strictly have to), mark this async, I think from a documentation POV, it is nice.
There was a problem hiding this comment.
I'm not sure what you mean.
- If you mean I should just add "async" in front of the method without any further changes, that would actually be misleading from the documentation POV. The method is fully synchronous, ESLint would complain about the missing "async" otherwise.
- If you mean rewrite it to make it asynchronous, we could do it by changing at the very end
return texturesReady;toawait texturesReady;. But I don't see any benefit in that. Since this is a public method, better leave the decision to the calling code. Async callers may await on the promise, sync callers can work with the promise.
Awaiting on every single texture promise is not an option either, cause the promises are generated sequentially, that would make the texture loading sequential.
There was a problem hiding this comment.
@atomicsulfate , I mean option 1.
Technically the function is synchronous, yes, but it returns a Promise, so the result isn't fully known when the function returns. You can argue that the function returns without any need to wait, but users of this are going to likely anyway await/.then() on the result of this function, so they won't know if it was synchronous or not, they will just use the result.
The docs on Mozilla.org also mention:
Async functions can contain zero or more await expressions.
Another benefit to using async is that any error thrown will cause the promise to reject, so users of the API can use .catch and react accordingly. Otherwise they need to wrap this function in a try/catch block, see: https://stackoverflow.com/questions/45594596/async-function-without-await-in-javascript#comment87236078_45594870
| // Check that texture promised is not yet resolved. | ||
| let textureLoaded = false; | ||
| const texturePromise = callbackSpy.firstCall.args[0]; | ||
| texturePromise.then(() => { |
There was a problem hiding this comment.
I guess this works because of the await wait below, but to be safe, you should await here too...? Or put the rest of the test in the .then() ?
There was a problem hiding this comment.
Wait... not here, that would break the test, but before await wait ?
There was a problem hiding this comment.
done, fixed similar cases in other tests as well.
Signed-off-by: Andres Mandado <andres.mandado-almajano@here.com>
Signed-off-by: Andres Mandado andres.mandado-almajano@here.com
TileGeometryCreator.createAllGeometries() returns now a promise that
is resolved when all textures in the tile are uploaded in GPU. This ensures that all textures are rendered on frame complete event.