fix: Track channel load status individually #344
Conversation
|
There was a problem hiding this comment.
Thinking out loud here: I wonder if, in the spirit of not duplicating state, hasChannelLoadedRef should be merged with channelVersions below now that it's also responsible for tracking per-channel load states? e.g. an initial load would be version -1? I'm okay letting this fix in as-is but might return to this in the future
I tested this and it seems to work great! I think it's initialized to version Edit: |
| @@ -232,7 +232,7 @@ const App: React.FC<AppProps> = (props) => { | |||
| }; | |||
|
|
|||
| const setAllChannelsUnloaded = (numberOfChannels: number): void => { | |||
| setChannelVersions(new Array(numberOfChannels).fill(0)); | |||
| setChannelVersions(new Array(numberOfChannels).fill(INITIAL_CHANNEL_VERSION)); | |||
| }; | |||
|
|
|||
| const setOneChannelLoaded = (index: number): void => { | |||
There was a problem hiding this comment.
Channel versions are incremented once they're loaded once!
There was a problem hiding this comment.
Looks like channel transfer functions are now getting recomputed rather than remapped on initial load. This is why I suggested -1 for INITIAL_CHANNEL_VERSION initially. Though it does add some additional complexity: setOneChannelLoaded has to go from simply incrementing channel versions to something like Math.max(version + 1, 1), and we need two different versions of setAllChannelsUnloaded for when channels are unloaded due to an image switch vs. due to loading new data from the same image, where the second does something like setChannelVersions(getChannelVersions().map((version) => Math.min(version, 0)));.
Not sure whether the additional complexity is worth the unification; I'm leaning towards yes, but I'm on the fence. If so, the representation of channelVersions should definitely get a few lines of documentation. (And honestly, it should already have documentation - the current representation had me confused for a bit just now, and I wrote it. The rationale for representing loaded state as a number rather than a boolean is described all the way back in #156, though I'm not sure if it's still necessary given all the smarter loading behaviors we've added in the meantime.)
There was a problem hiding this comment.
Moved this to a new branch because of some complexity that was worth checking a little more thoroughly! It's now on refactor/remove-channel-loaded-ref.
There was a problem hiding this comment.
I'm all for any change that both makes things work correctly and improves the readability, maintainability, and simplicity of the codebase. Do you really think we can get rid of the channelversions?
ShrimpCryptid
force-pushed
the
fix/load-disabled-channels
branch
from
c5cdbfe to
e728061
Compare
Problem
Fixes #342, "Volumes that are initially disabled are remapped incorrectly when enabled."
The issue seemed to be caused by the
initialLoadRefonly allowing normal initialization of channels for enabled channels were loaded for the first time. Channels that were initially disabled, but then enabled later, would have the initial default ramps or control points remapped to incorrect values.This change fixes the issue by changing
initialLoadRefto be a boolean array, and the loaded status of each channel is tracked individually. This way, the normal initialization can occur even when a channel is loaded later.From testing it seems like the bug was introduced in #338, but I can't tell exactly what changes to the viewer would have broken this behavior.
Estimated review size: tiny, <5 minutes
Solution
channelLoadedRef, and checks for initial load are performed against it.Type of change
Please delete options that are not relevant.
Steps to Verify:
Screenshots (optional):