template changes #15
Open
template changes #15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pgrit
requested changes
Oct 22, 2025
| } | ||
|
|
||
| /// <summary> | ||
| /// Calculates the absolute Difference from two equal-sized images with equal channel count |
There was a problem hiding this comment.
Suggested change
| /// Calculates the absolute Difference from two equal-sized images with equal channel count | |
| /// Calculates the absolute difference |a - b| of two equal-sized images with equal channel count |
Comment on lines
+216
to
+223
| /// <summary> | ||
| /// Returns a image from the list images | ||
| /// </summary> | ||
| /// <param name="index"></param> | ||
| /// <returns></returns> | ||
| public (string Name, Image Image, DataType TargetType, InitialTMO TMOOverride) GetImage(int index) { | ||
| return images[index]; | ||
| } |
There was a problem hiding this comment.
Instead of the long tuple, it would be better to define a struct that contains name, image, etc and return that here.
Instead or in addition to GetImage this could be an indexer.
Sth like
public struct ImageEntry
{
public string Name;
// ...
}
public ImageEntry this[int index] => images[index];
Comment on lines
+295
to
+303
| /// <summary> | ||
| /// Sets the key of this flipbook to keep track on the react side of all flipbooks | ||
| /// Same thing as a group name but this key can decide which flipbook of a group fired a listener to the c# side | ||
| /// </summary> | ||
| /// <param name="key">Key for the dictionary on React side</param> | ||
| public FlipBook SetKey(int[] key) { | ||
| this.key = key; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Unclear what this does. Since this is public API, we'll need to define what the purpose of the key is and why it is an array of integers.
Does it need a specific convention? -- Define
Is it arbitrary and up to the user? -- Why the int array instead of sth more intuitive like a string?
Comment on lines
+436
to
+437
| if (String.IsNullOrEmpty(this.containerId)) | ||
| this.containerId = id; |
There was a problem hiding this comment.
Why do you only store it the first time?
Seems like this could yield confusing behavior if the same flip book is generated multiple times.
Comment on lines
+557
to
+575
|
|
||
| // string json = $$""" | ||
| // { | ||
| // "width": {}, | ||
| // "height": {}, | ||
| // "containerId": "{}", | ||
| // "initialZoom": {}, | ||
| // "initialTMO": {}, | ||
| // "initialTMOOverrides": [ | ||
| // {} | ||
| // ], | ||
| // "names": [{{string.Join(',', nameStrs.Select(n => $"\"{n}\""))}}], | ||
| // "dataUrls": [{{string.Join(',', dataStrs.Select(n => $"\"{n}\""))}}], | ||
| // "types": [{{string.Join(',', typeStrs.Select(n => $"\"{n}\""))}}], | ||
| // "colorTheme": "{}", | ||
| // "groupName": "{{groupName}}", | ||
| // "hideTools": {} | ||
| // } | ||
| // """; |
There was a problem hiding this comment.
Suggested change
| // string json = $$""" | |
| // { | |
| // "width": {}, | |
| // "height": {}, | |
| // "containerId": "{}", | |
| // "initialZoom": {}, | |
| // "initialTMO": {}, | |
| // "initialTMOOverrides": [ | |
| // {} | |
| // ], | |
| // "names": [{{string.Join(',', nameStrs.Select(n => $"\"{n}\""))}}], | |
| // "dataUrls": [{{string.Join(',', dataStrs.Select(n => $"\"{n}\""))}}], | |
| // "types": [{{string.Join(',', typeStrs.Select(n => $"\"{n}\""))}}], | |
| // "colorTheme": "{}", | |
| // "groupName": "{{groupName}}", | |
| // "hideTools": {} | |
| // } | |
| // """; |
Comment on lines
+303
to
+304
| var relX = event.nativeEvent.offsetX;: nativeEvent. | ||
| var relY = event.nativeEvent.offsetY;: nativeEvent. |
Comment on lines
+282
to
+287
| if (this.props.onWheel) | ||
| { | ||
| this.props.onWheel(event.deltaY); | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
Not tested, but sounds like quite awkward UX to have ALT as a means to override the zoom behavior but also then forward this wheel event in the flip book context?
Comment on lines
+152
to
+165
| let work: Promise<Float32Array | HTMLImageElement>[] = []; | ||
| for (let i = 0; i < data.dataUrls.length; ++i) { | ||
| let loadFn; | ||
| switch (data.types[i]) { | ||
| case ImageType.Single: loadFn = readFloat; break; | ||
| case ImageType.Half: loadFn = readHalf; break; | ||
| case ImageType.Rgbe: loadFn = readRGBE; break; | ||
| case ImageType.Ldr: loadFn = readLDR; break; | ||
| case ImageType.Float32Array: loadFn = (data: Float32Array) => data; break; | ||
| default: console.error(`unsupported type: ${data.types[i]}`); | ||
| } | ||
| work.push(loadFn(data.dataUrls[i])); | ||
| } | ||
| let images = await Promise.all(work); |
There was a problem hiding this comment.
this code duplication should be eliminated
Comment on lines
+170
to
+176
| const book = ref.current; | ||
|
|
||
| if(!book) | ||
| continue; | ||
|
|
||
| const tms = book.props.toneMappers; | ||
| tms[book.state.selectedIdx].setPixels(images[0] as (Float32Array<ArrayBufferLike>)); |
There was a problem hiding this comment.
Can you safely assume f32 data here?
| tms[book.state.selectedIdx].setPixels(images[0] as (Float32Array<ArrayBufferLike>)); | ||
| } | ||
|
|
||
| // flipBook.imageContainer.current.props.rawPixels[flipBook.state.selectedIdx] = images[0] as (Float32Array<ArrayBufferLike>); //TODO: maybe it doesnt work like that |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.