Skip to content

feat: always store volume rendering depth samples in JSON state if volume rendering is enabled#854

Open
seankmartin wants to merge 6 commits intogoogle:masterfrom
MetaCell:feat/bump-default-volume-rendering-samples
Open

feat: always store volume rendering depth samples in JSON state if volume rendering is enabled#854
seankmartin wants to merge 6 commits intogoogle:masterfrom
MetaCell:feat/bump-default-volume-rendering-samples

Conversation

@seankmartin
Copy link
Copy Markdown
Contributor

@seankmartin seankmartin commented Oct 21, 2025

Always store the number of depth samples from volume rendering in the JSON state if volume rendering is enabled. Now the JSON state should contain the number of volume rendering samples if it was either changed from the default, or never changed from the default, but volume rendering is active.

This should allow us to make future changes to the volume rendering number of samples more readily without affecting existing states that have volume rendering in use.

@jbms
Copy link
Copy Markdown
Collaborator

jbms commented Oct 21, 2025

I'd be inclined to bump the default for new layers but not modify existing states.

@seankmartin seankmartin changed the title feat: bump default volume rendering samples feat: always store volume rendering depth samples in JSON state if volume rendering is enabled Nov 4, 2025
@seankmartin seankmartin marked this pull request as ready for review November 4, 2025 11:41
@seankmartin
Copy link
Copy Markdown
Contributor Author

Thanks @jbms, I changed the scope of this PR to instead focus on always storing the number of depth samples from volume rendering in the JSON state if volume rendering is enabled. So now the JSON state should contain the number of volume rendering samples if it was either changed from the default, or never changed from the default, but volume rendering is active.

This should allow us to make future changes to the volume rendering number of samples more readily without affecting existing states that have volume rendering in use

@chrisj
Copy link
Copy Markdown
Contributor

chrisj commented Feb 26, 2026

On preserving state behavior, should we consider removing TrackableValue's return of undefined when the value is the same as the default? State size is one concern. If trackablevalue.tojson had a force argument, it would make this PR a little cleaner

return this.volumeRenderingDepthSamplesTarget.toJSON(
  isVolumeRenderingActive,
);

@seankmartin
Copy link
Copy Markdown
Contributor Author

seankmartin commented Feb 27, 2026

On preserving state behavior, should we consider removing TrackableValue's return of undefined when the value is the same as the default? State size is one concern. If trackablevalue.tojson had a force argument, it would make this PR a little cleaner

return this.volumeRenderingDepthSamplesTarget.toJSON(
  isVolumeRenderingActive,
);

That's a great idea for the force parameter thank you Chris. Maybe it could be configurable at compile time the default force value and then up to the app whether to store values always or only if non default?

Though a lot of classes just implement being a Trackable so this would need to be enforced quite broadly across the codebase

@chrisj
Copy link
Copy Markdown
Contributor

chrisj commented Feb 27, 2026

Good point, would require a lot of changes, for example

return undefined;

Would need to add a !force check where it returns undefined

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.

3 participants