Misc changes and helpers from 3d-tiles #5249

lilleyse · 2017-04-27T20:09:06Z

Some of the bigger things here:

JobScheduler that is used to load WebGL resources for models (just models for now)
TileBoundingBox is now TileBoundingRegion to fit the 3d-tiles convention. The other bounding volume types TileBoundingSphere and TileOrientedBoundingBox are brought over but not currently used.
Texture stats
Model memory stats
Some simplification to Transforms, but no difference in behavior.

What it doesn't have

RequestScheduler changes
Anything specifically 3d-tiles related.

pjcozzi · 2017-04-27T22:06:18Z

Could we please keep the git history?

bagnell · 2017-04-27T23:09:28Z

This looks good to me.

Is the plan to move all WebGL resource creation to use the JobScheduler? Should be easy for Primitive and GroundPrimitive but more work for billboards/points/labels and polyline collection.

lilleyse · 2017-04-27T23:50:48Z

Could we please keep the git history?

Won't the history still be preserved when 3d-tiles is merged in?

pjcozzi · 2017-04-28T15:10:13Z

Won't the history still be preserved when 3d-tiles is merged in?

I was thinking the same thing, but I am not 100% sure, could you test in a small example?

mramato · 2017-04-28T15:12:33Z

I was thinking the same thing, but I am not 100% sure, could you test in a small example?

There's no need to test, yes history will be preserved. There will be a merge commit into 3d-tiles that resolves the dupes, but if you were to ever go back and chronologically determine where code came from, the original 3d-tiles commits will be the first.

mramato · 2017-04-28T16:07:57Z

Even if everyone approves this PR (I haven't looked at it yet), I'd prefer we hold off on merging this until after the release on Monday. Thanks.

mramato · 2017-04-28T16:09:31Z

Is JobScheduler ultimately meant to be used at all layers (i.e. if I have a large CZML loading task that I can split into work units, can I use the JobScheduler to distribute that over multiple frames?) Or is it only meant for WebGL resources?

lilleyse · 2017-04-28T16:22:13Z

Is the plan to move all WebGL resource creation to use the JobScheduler? Should be easy for Primitive and GroundPrimitive but more work for billboards/points/labels and polyline collection.

Model served as a testing ground and it would great to expand it to everything.

Is JobScheduler ultimately meant to be used at all layers (i.e. if I have a large CZML loading task that I can split into work units, can I use the JobScheduler to distribute that over multiple frames?) Or is it only meant for WebGL resources?

That seems to be the ultimate goal. More discussion here (plus a similar comment from you) #2655

mramato · 2017-04-28T16:41:30Z

Good to hear, since it's marked private I'm fine with it going in as-is and then being refactored as needed to meet all of our use cases.

mramato · 2017-04-28T19:05:22Z

I haven't reviewed this in its entirety yet, but just a couple of thoughts I have to spur conversation:

Doesn't having the JobScheduler be scene-specific introduce issues with having multiple Scenes on the same page (i.e. they will not cooperate and chew into each other's time). Shouldn't this be a singleton (perhaps it keeps track of which jobs are in which scenes under the hood so that it can be fair to each?
Also, doesn't this more tightly couple worker tasks to frame rate? i.e. I think we ultimately want to go in the opposite direction so that fps doesn't affect other types of work being done (though they will always be related obviously).
We have a problem now where some things don't behave as expected unless added to the scene (because they are locked to rendering). Tieing the job schedule to the scene makes it hard to use in places that need to split up work but are in no way tied to rendering, for example loading a KML file as discussed in Defer processing of KML to allow for smoother performance #3970. This is slightly related to one and in my opinion even more of a reason that JobSchedule should be a singleton decoupled from the scene (but still used by the scene).

mramato · 2017-05-01T15:59:55Z

Source/Core/joinUrls.js

@@ -55,6 +57,9 @@ define([
        var baseUri = first;
        if (second.isAbsolute()) {
            baseUri = second;
+            if (second.scheme === 'data') {


What is this for? It's probably worth it to have a comment here, since the purpose of this isn't immediately obvious.

mramato · 2017-05-01T16:01:56Z

Source/Scene/JobType.js

+        PROGRAM : 1,
+        BUFFER : 2,
+        NUMBER_OF_JOB_TYPES : 3
+        // Potential additional types:


This smells like a TODO, even if the word isn't there. Should we write an issue for this and remove the comment?

mramato · 2017-05-01T16:03:51Z

Source/Scene/TileBoundingSphere.js

+        Primitive) {
+    'use strict';
+
+    function TileBoundingSphere(center, radius) {


Missing documentation (even if it's private, we should still document it).

mramato · 2017-05-01T16:04:50Z

Source/Scene/TileOrientedBoundingBox.js

+        Primitive) {
+    'use strict';
+
+    function TileOrientedBoundingBox(center, halfAxes) {


More missing doc.

mramato · 2017-05-01T16:06:43Z

I know a lot of the updates here are private, but is there anything that warrants an update to CHANGES.MD?

mramato · 2017-05-01T16:10:51Z

Sorry to be "that guy", but this should have really been multiple PRs, it jams way to many changes together.

That being said, other than my concerns over the JobScheduler the rest of this PR looks okay to me (at least the parts I'm qualified to review). Not sure if anyone else wants to take a look at this.

I'd prefer we not merge this until the impact of JobScheduler is discussed further and better understood.

lilleyse · 2017-05-01T17:18:19Z

Source/Scene/Fog.js

@@ -131,7 +131,7 @@ define([

        // Fade fog in as the camera tilts toward the horizon.
        var positionNormal = Cartesian3.normalize(camera.positionWC, scratchPositionNormal);
-        var dot = CesiumMath.clamp(Cartesian3.dot(camera.directionWC, positionNormal), 0.0, 1.0);
+        var dot = Math.abs(Cartesian3.dot(camera.directionWC, positionNormal));


This change was from this comment: #4307 (comment). The previous math resulted in fog being fully applied unless looking straight at the horizon or at the sky.

lilleyse · 2017-05-01T18:09:17Z

Doesn't having the JobScheduler be scene-specific introduce issues with having multiple Scenes on the same page (i.e. they will not cooperate and chew into each other's time). Shouldn't this be a singleton (perhaps it keeps track of which jobs are in which scenes under the hood so that it can be fair to each?

Yeah this is a problem. The request scheduler is a singleton but has a similar problem. They both operate on budgets that need to be reset at every frame. Where would be the best place to reset them in app that contains multiple scenes?

lilleyse · 2017-05-01T18:10:12Z

Also, doesn't this more tightly couple worker tasks to frame rate? i.e. I think we ultimately want to go in the opposite direction so that fps doesn't affect other types of work being done (though they will always be related obviously).

It definitely has a rendering bias.. I'll add some of these thoughts to the issue.

lilleyse · 2017-05-01T18:12:44Z

Addressed most of the other comments.

bagnell · 2017-05-01T18:25:39Z

Doesn't having the JobScheduler be scene-specific introduce issues with having multiple Scenes on the same page (i.e. they will not cooperate and chew into each other's time). Shouldn't this be a singleton (perhaps it keeps track of which jobs are in which scenes under the hood so that it can be fair to each?

Yeah this is a problem. The request scheduler is a singleton but has a similar problem. They both operate on budgets that need to be reset at every frame. Where would be the best place to reset them in app that contains multiple scenes?

When investigating the design for multiple viewports, there was a bunch of stuff in scene that should operate on all scenes. I think pulling this out of Scene would be part of that work. It would be different for multiple canvases which has its own problems. We still need WebGL support for a shared context there though.

pjcozzi · 2017-05-08T19:27:32Z

@lasalvavida heads up that the changing to Model.js here will conflict with your work in #4808. The merge shouldn't be too bad. Can you please proactively merge this branch into your branch even before this PR is merged?

pjcozzi · 2017-05-08T19:34:13Z

For the Job Scheduler, let's continue the conversation in #2655. We can incorporate changes as we adapt it for more parts of Cesium. Since it is private and is working sufficiently for 3D Tiles, I'm not sure that we should hold up this PR for more architecture work.

Multiple scenes may be first on the list if it becomes a problem in practice.

Is there anything else this PR is waiting on?

mramato · 2017-05-08T19:36:36Z

Where would be the best place to reset them in app that contains multiple scenes?

That's a good question. Right now we don't really have that concept. (Which is why one of the options @bagnell was exploring about for multiple viewports was the idea of a SceneManager or SceneCollection class. I don't think we need something that heavyweight to satisfy this initial case though. One option is to have an automatically managed global shared state (via Scene constructor/destroy) and have some logic based on that to decide if/when any global action needs to be taken.

I need to investigate more into it, but another possibility is that it will turn out that requireAnimationFrame uses the same timestamp for all scene instances per-frame, in which case we could simply cache the last time and only update when different. I need to write a test case to explore this behavior though.

mramato · 2017-05-08T19:49:47Z

Is there anything else this PR is waiting on?

Have we tested this with multiple Viewer instances to makes sure it doesn't break anything ? That is my only showstopper concern.

From a development standpoint, I would have liked to have seen JobScheduler discussed more up front and at a holistic view for Cesium. I'm concerned that it's going to have to be rewritten to account for things like DataSource loading which could have just been added from the get go with a little more forethought. But I've said my peace and I'll leave it at that.

lilleyse · 2017-05-08T20:13:41Z

Ignoring #5226, I tested that multiple viewer instances works with the job scheduler.

mramato · 2017-05-08T20:24:56Z

Thanks.

Misc changes and helpers from 3d-tiles

3f35a40

lilleyse mentioned this pull request Apr 27, 2017

Master to 3D Tiles Cleanup #5250

Merged

mramato reviewed May 1, 2017

View reviewed changes

Data uri fixes and testing

b104c27

lilleyse commented May 1, 2017

View reviewed changes

lilleyse mentioned this pull request May 1, 2017

Unified time slicing tasks #2655

Open

Doc and other fixes

1dd7864

pjcozzi merged commit cafc530 into master May 8, 2017

pjcozzi deleted the from-3d-tiles branch May 8, 2017 22:11

hpinkos mentioned this pull request May 18, 2017

Fix viewerCesiumInspectorMixin #5324

Merged

xtassin mentioned this pull request Dec 2, 2017

From 1.37, GLTF shader paths are relative, even when starting with a slash #5844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc changes and helpers from 3d-tiles #5249

Misc changes and helpers from 3d-tiles #5249

lilleyse commented Apr 27, 2017

pjcozzi commented Apr 27, 2017

bagnell commented Apr 27, 2017

lilleyse commented Apr 27, 2017

pjcozzi commented Apr 28, 2017

mramato commented Apr 28, 2017

mramato commented Apr 28, 2017

mramato commented Apr 28, 2017

lilleyse commented Apr 28, 2017

mramato commented Apr 28, 2017

mramato commented Apr 28, 2017

mramato May 1, 2017

mramato May 1, 2017

mramato May 1, 2017

mramato May 1, 2017

mramato commented May 1, 2017

mramato commented May 1, 2017

lilleyse May 1, 2017

lilleyse commented May 1, 2017

lilleyse commented May 1, 2017

lilleyse commented May 1, 2017

bagnell commented May 1, 2017 •

edited

Loading

pjcozzi commented May 8, 2017

pjcozzi commented May 8, 2017

mramato commented May 8, 2017

mramato commented May 8, 2017

lilleyse commented May 8, 2017

mramato commented May 8, 2017

Misc changes and helpers from 3d-tiles #5249

Misc changes and helpers from 3d-tiles #5249

Conversation

lilleyse commented Apr 27, 2017

pjcozzi commented Apr 27, 2017

bagnell commented Apr 27, 2017

lilleyse commented Apr 27, 2017

pjcozzi commented Apr 28, 2017

mramato commented Apr 28, 2017

mramato commented Apr 28, 2017

mramato commented Apr 28, 2017

lilleyse commented Apr 28, 2017

mramato commented Apr 28, 2017

mramato commented Apr 28, 2017

mramato May 1, 2017

Choose a reason for hiding this comment

mramato May 1, 2017

Choose a reason for hiding this comment

mramato May 1, 2017

Choose a reason for hiding this comment

mramato May 1, 2017

Choose a reason for hiding this comment

mramato commented May 1, 2017

mramato commented May 1, 2017

lilleyse May 1, 2017

Choose a reason for hiding this comment

lilleyse commented May 1, 2017

lilleyse commented May 1, 2017

lilleyse commented May 1, 2017

bagnell commented May 1, 2017 • edited Loading

pjcozzi commented May 8, 2017

pjcozzi commented May 8, 2017

mramato commented May 8, 2017

mramato commented May 8, 2017

lilleyse commented May 8, 2017

mramato commented May 8, 2017

bagnell commented May 1, 2017 •

edited

Loading