Skip to content
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

feat(datasource/graphene) auto reloading graphene manifest after an edit #465

Merged
merged 5 commits into from
Oct 20, 2023

Conversation

chrisj
Copy link
Contributor

@chrisj chrisj commented May 17, 2023

Replaces #450

With our current infrastructure, the only way for us to get mesh updates is to continuously poll the manifest endpoint to see if it changes, we can't rely on 404 responses as previously thought. Eventually we want to get push messages on the client to replace this logic.

I was asked to try to reduce the flicker that occurs when we change the manifest of an existing cell. I have partial "fix" with this commit 80d95e1. The issue is that the ManifestChunk arrives before the first FragmentChunk.

The easiest solution that mostly works is to prevent ManifestChunks from triggering redraws. I was surprised that ManfestChunk gets a GPU_MEMORY state. It still can cause issues.

Do you have any suggestions on how we could make it smoother. I was asked to go even further by keeping existing fragments from previous cells around until the new cell is at least partially meshed. For example, with a merge, it is very easy to just combine the fragments from each segment.

@fcollman
Copy link
Contributor

@jbms any reason we can't get this one merged?

@@ -949,6 +949,27 @@ class GraphConnection extends SegmentationGraphSourceConnection {
return undefined;
}

getMeshSource() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should somehow ensure that it is associated with this graph --- can have more than one datasource in a UserLayer.

Copy link
Contributor Author

@chrisj chrisj Oct 2, 2023

Choose a reason for hiding this comment

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

Would this solve that? It looks like I could either do loadState.dataSource.subsources or loadState.subsources

getMeshSource() {
    const {layer} = this;
    for (const dataSource of layer.dataSources) {
      const {loadState} = dataSource;
      if (loadState instanceof LoadedLayerDataSource) {
        const {subsources} = loadState.dataSource;
        const graphSubsource = subsources.filter(subsource => subsource.id === 'graph')[0];
        if (graphSubsource?.subsource.segmentationGraph !== this.graph) {
          continue;
        }
        const meshSubsource = subsources.filter(subsource => subsource.id === 'mesh')[0];
        if (meshSubsource) {
          return meshSubsource.subsource.mesh;
        }
      }
    }
    return undefined;
  }

chunk.copyToGPU(this.gl);
visibleChunksChanged = true;
if (chunk.constructor.name !== "ManifestChunk") {
visibleChunksChanged = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be done by name, and also skipping the notification doesn't really fix the problem in general, because any other chunk update would also trigger the notification. Additionally it will still redraw on the first fragment that is received so there will still be flicker if there is more than one fragment. I think a different strategy is needed to avoid flicker.

Will some of the existing fragments be referenced by the new manifest, if they haven't changed, or does the fragment key depend on the root id?

@jbms
Copy link
Collaborator

jbms commented Sep 19, 2023

Sorry for the delay in reviewing, I added some comments.

@chrisj
Copy link
Contributor Author

chrisj commented Oct 11, 2023

@jbms I added a response about getMeshSource

I will remove the attempt at fixing flicker until we have a good solution

@chrisj
Copy link
Contributor Author

chrisj commented Oct 19, 2023

@jbms I pushed the change I talked about and removed the flicker reduction code so it should be ready to be merged.

@jbms jbms merged commit d5d8b6d into google:master Oct 20, 2023
20 checks passed
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.

None yet

3 participants