Skip to content

Shared Context #12635

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

Merged
merged 36 commits into from
Jul 28, 2025
Merged

Shared Context #12635

merged 36 commits into from
Jul 28, 2025

Conversation

pmconne
Copy link
Contributor

@pmconne pmconne commented May 26, 2025

Description

Currently every Scene has its own WebGL context. Two scenes cannot share primitives, shader programs, textures, etc. This can cause an app that displays multiple scenes to use more memory than it should need to, especially if it's displaying large primitives like 3D tilesets.

This PR is a quick and dirty attempt to enable multiple scenes to share a single WebGL context. This feature is opt-in; the existing default behavior and APIs are undisturbed.

The new APIs are @Private for now until we resolve (in subsequent PRs) various open questions enumerated below.

Reference-counting in PrimitiveCollection

The constructor now takes a new option countReferences (default false). Adding a Primitive to any reference-counted PrimitiveCollection increments its reference count. Removing one decrements the count and - if destroyPrimitives is true and the reference count reaches zero - destroys the primitive.

SharedContext

Apps can create a SharedContext from optional ContextOptions. It wraps a Context. It can be supplied in place of ContextOptions to the constructors of Scene, CesiumWidget, and Viewer. In that case, its primitives will be reference-counted and its _context will be a proxy that mostly delegates to the shared context.

By default, a SharedContext is destroyed automatically after every Scene that uses it is destroyed.

Issue number and link

Testing plan

Added a "Shared Context" sandcastle that displays a single tileset and cylinder primitive in two viewers sharing a WebGL context. Commenting out the line that enables context sharing illustrates how this previously did not work (the graphics would only display in the first viewer).

Added unit tests.

Open questions

  • Many bits of code rely on FrameState.frameNumber when updating. This breaks when multiple contexts are using the same object. See for example the change I made to ImageBasedLighting.
  • Some additional Context fields probably should be unique per-Scene rather than forwarded to the shared context (e.g., _id, _nextPickColor?)
  • Some primitives like Cesium3DTileset store state from previous traversals, assuming only a single Scene is using them. (This also precludes traversal for purposes other than display).
  • There doesn't appear to be a way to draw the same tileset with two different styles in two different scenes.
  • The higher-level EntityCollection/DataSourceDisplay APIs generate primitives under the hood, making it tricky to share WebGL resources between entities even if the underlying scenes share the same context.
  • Scene.initializeFrame does some housekeeping on the context's shader and texture caches every 12- frames - that's going to thrash a bit when multiple scenes share the context.
  • Map/globe do not appear to share WebGL resources between contexts.
  • A tile's content and the styling applied to it via Cesium3DTilesetStyle are tightly coupled, making it tricky to apply different styling to the same tileset in different views.

Really the most fundamental issue is that primitives and their WebGL resources do not really live independently from a Context that wants to display them. WebGL resources instead get created as a by-product of trying to render a primitive, within the context in which they are being rendered.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change no public API changes
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @pmconne!

✅ We can confirm we have a CLA on file for you.

@pmconne
Copy link
Contributor Author

pmconne commented May 29, 2025

Here are a few slides describing the approach, potential next steps, and proposed benefits.

I'm looking for feedback re: feasibility from CesiumJS experts familiar with all the nuances I've undoubtedly overlooked.

@javagl
Copy link
Contributor

javagl commented May 31, 2025

I cannot provide a thorough review (or at least, would have to allocate more time for that).

A high-level comment is that I'm in strong favor of whatever helps to clean up the resource handling. There is a tight coupling between 'Something' and 'Something, but rendered'. I think that trying to use something in two contexts will eventually lead to a state where the separation between "an object" and "what is required for rendering that object" becomes clearer.

(It is not unusual that generalizations - like ~"having two scenes", forces cleaner approaches. Although it's usually not "two". Musings about "two"......).

Similarly, when you mention that "Some primitives like Cesium3DTileset store state from previous traversals...", one could argue that anything that is related to the traversal is not part of the inherent state of a tileset, but rather of that of a 'TilesetTraverser', but "traversal" and "rendering" are similarly coupled (and be it only via the FrameState)

An overly specific question is whether #1567 (comment) 👴 might be resolved by the reference counting. I might try to allocate some time to try it out...

@ggetz
Copy link
Contributor

ggetz commented Jun 3, 2025

Thanks for the proposal @pmconne!

Overall, I'm very pleased with how simple the underlying approach is here. I can't think of any particular reason not to take this route. I expect the brunt of the effort will be testing and updating the existing feature set to take advantage of a shared context, and refactoring where existing assumptions are not sufficient.

As @javagl mentioned, this is probably a chance to update some older parts of the codebase for the better.

@lilleyse Could you please take a high-level pass on the approach here?

Assuming @lilleyse does not identify any show-stoppers, we should discuss a plan for how we should go about iterating on this for additional features.

@ggetz
Copy link
Contributor

ggetz commented Jun 3, 2025

CC #5214

@lilleyse
Copy link
Contributor

  • Does this approach work with different scene modes, e.g. one scene using 2D and another using 3D? I think it should work since most primitives generate separate 2D and 3D geometry / commands rather than rebuilding when the mode changes.

Any other scene state that potentially affects geometry / shaders?

@pmconne
Copy link
Contributor Author

pmconne commented Jun 27, 2025

Mark the new APIs @Private for now.

@ggetz
Copy link
Contributor

ggetz commented Jun 27, 2025

Following up from chatting with @pmconne offline:

  • We know the 3D Tiles selection for multiple cameras is going to be critical for making this feature broadly useful, but it's likely a sizable chunk of work. We should coordinate with the Cesium native team who have implemented this and see what we can bring back to CesiumJS.
  • In the meantime, to keep the scope of this PR small, we can mark the opt-in APIs as private for now to get this merged into main, then incrementally merge in improvements like 3D Tiles selection or others we identify.
  • When we're ready to broadly launch the feature, we can mark the opt-in APIs public (and make sure we have polished docs and examples to support that).

@pmconne
Copy link
Contributor Author

pmconne commented Jul 7, 2025

  • I have updated CHANGES.md with a short summary of my change

@ggetz I presume that new @Private APIs don't bear mentioning in this change log?

@pmconne pmconne changed the title [PoC] Shared Context Shared Context Jul 7, 2025
@ggetz
Copy link
Contributor

ggetz commented Jul 7, 2025

@pmconne

I presume that new @Private APIs don't bear mentioning in this change log?

Correct. The change log should include only public API changes.

return canvas;
case "destroy":
return destroy;
// ###TODO: When will this be inaccurate? device pixel ratio? Canvas larger than maximum drawing buffer dimensions supported by WebGL implementation?
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to address this now, or in a later PR?
We typically don't merge code into main with TODO comments, and prefer to document them in an issue if they are not addressed.

The max viewport dimensions are available in ContextLimits.maximumViewportWidth/ContextLimits.maximumViewportHeight.

Pixel ratio is less straightforward, as it's "owned" by Scene.frameState. Perhaps it should live in Context instead.

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 discussed here, pixel ratio is tricky and not 100% solved. An API to obtain the dimensions of a canvas (or other element) in integer device pixels solves the problem, except it's not yet supported in Safari.

iTwin.js attempts to account for DPR here.

Choose a reason for hiding this comment

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

I created #12762 to track this outside the code / this PR. I propose removing this TODO from this code, resolving this conversation, possibly merging this PR, and then scheduling some work on that issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Choose a reason for hiding this comment

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

@ggetz Feel free to resolve your comment here if you are satisfied with the above plan.

@ggetz
Copy link
Contributor

ggetz commented Jul 28, 2025

Thanks @pmconne and @markschlosseratbentley! This looks ready to go. We'll follow up in #5214 for additional updates.

@ggetz ggetz added this pull request to the merge queue Jul 28, 2025
Merged via the queue into main with commit c540202 Jul 28, 2025
12 of 13 checks passed
@ggetz ggetz deleted the pmc/shared-context branch July 28, 2025 13:07
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.

5 participants