-
Notifications
You must be signed in to change notification settings - Fork 293
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
Tile creation and update #71
Conversation
|
Open question: should getStatus return an array of statuses instead of just a string (e.g. ["geometry", "imagery"])? This way we could launch parallel commands, be in a ready state (displayable) without having to load everything, etc. |
|
…ing promises. Node.pending no longer needs manual modification
I added a callback after each command execution. By default, it only sets the pending attribute to false, but it can be easily be expanded by passing a function to InterfaceCommander.request(). ...
var cb = function(type, id) { return function() {console.log(type, id);};};
this.interCommand.request(updateType, node, layer, {callback: cb(updateType, node.id)}); Result:
Can be handy for debugging. I still want to change something before merging with the master. Each time a promise is resolved, I have to test whether a tile has been disposed of or not (in the time a server request is completed, the tile may have been deleted). This means I have to add a test each time I rely on promises, for example, in TileProvider.js: return this.providerElevationTexture.getElevationTexture(tile.tileCoord, elevationlayerId).then(function(terrain) {
if(this.disposed) return;
this.setTextureElevation(terrain);
}.bind(tile)); I'd like this process to be automated, so when implementing a new Provider, the programmer doesn't have to worry about it. Does anyone have an idea on how to do this elegantly (maybe @tbroyer )? |
…atus() to allow concurrent requests (for imagery and elevation for instance)
I'd like to have some feedback on the PR. Most of the developers will be impacted by this work since it changes the way providers and tiles interact, so it is important for the project as a whole that other developers share their opinion on this matter. Latest development: Changed getStatus() to return an array of data types. Added a ready() function to the node to separate data request (from getStatus) and readiness. I restored WMTS imagery, but I had to disable texture upscaling in the process (using the parent's texture while waiting for the right resolution to load). However, I think this part of the code needs to be changed (and moved out of WMTS_provider) because it seemed overly complicated. I'm still trying to find a way to remove the cullable attribute and to solve the issue mentionned in my previous comment. |
ping @gchoqueux @vmora @qdnguyen @nosy-b for feedback on what is basically the new low-level API. |
if(command) { | ||
var task = this.providerMap[command.layer.id].executeCommand(command); | ||
task = task.then(command.callback); | ||
arrayTasks.push(task); |
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.
one liner ?
arrayTasks.push(this.providerMap[command.layer.id].executeCommand(command).then(command.callback));
seems reasonable. |
8691137
to
f202514
Compare
@gchoqueux @qdnguyen @nosy-b Any update on reviewing this ? @peppsac will have a look at it soon too, to get it finally merged |
Still a few issues I'm working on. There are a few regression that need fixing, most notably the elevation not loading when zoomed in a lot. |
Didn't check deeply the modifications yet but It looked pretty slow compare to the master when we tried yesterday with @gchoqueux and as you mentioned, elevation was not loaded closer to the ground. |
I'm going to rebase this PR on master, then I'll work on fixing the 2 identified regressions (elevation and slowness). |
@Jeremy-Gaillard maybe we could close this PR as most of its content/ideas has been dispatched in smaller PR? |
Not ready for pull yet. Pull request for visibility, since quite a lot of things are changing.
Solve (at least partially) issues #70, #54 and #57
Content:
How to handle tile loading:
"none" or undefined indicate that no data is needed, "ready" indicates the tile is ready to be rendered.Any string will result in the creation of a new command, an array of string will result in the creation of multiple commands. getStatus() should return only the types of the data that it can currently load (e.g. don't return "elevation" if you don't have the underlying geometry)Important tile attributes:
pending: automatically set to true when a command is created. Must be set to false when the expected data is added to the tileHandled by the interface commanderdivided: the tile has been divided (refined). Set to true by the Quadtree classloaded: tile is ready to be displayedExample: see TileProvider.js, TileMesh.js and NodeProcess.js
TODO list:
Call browse() when a tile has been updated (loading is stuck until the camera moves otherwise)Fixed, I wasn't using the providers correctly (not returning promises)Why these changes?
Only pending is a bit out of place (in interfaceCommander and tile classes, maybe this could be solved by using a callback after each command is executed)Fixed using callbacks. This kind of separation helps when trying to understand what is going wrong and where when debuggingEdit: If you want to test the branch, you will see that there are still problems with the texturing of the globe. There are weird things happening with the UVs of the texture that I didn't quite investigate yet. I temporarily fixed it by modifying the shader (which I didn't commit). I attached the diff if you want to try the branch: shader.txt