Validate animation job bounding box #7883
Merged
Conversation
This file contains 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
MichaelBuessemeyer
requested changes
Jun 14, 2024
There was a problem hiding this comment.
Your fix works 👍
But please consider my suggestion: I idenfied a scenario in which case nothing of the color layer would be rendered but no warning would be shown.
I'd say to also fix this scenario before merging. I would pull the interect opertion out of the 'selectMagForTextureCreation` method and already pass the intersected bbox to the funciton
| ): [Vector3, number] { | ||
| // Utility method to determine the best mag in relation to the dataset size to create the textures in the worker job | ||
| // We aim to create textures with a rough length/height of 2000px (aka target_video_frame_size) | ||
| const colorLayerBB = new BoundingBox( | ||
| computeBoundingBoxFromBoundingBoxObject(colorLayer.boundingBox), | ||
| ); | ||
| const bb = new BoundingBox(boundingBox).intersectedWith(colorLayerBB); | ||
| const bb = boundingBox.intersectedWith(colorLayerBB); |
There was a problem hiding this comment.
Here the selected bbox is intersected with the color layer to get the actual rendered bbox, right?
| @@ -167,7 +168,14 @@ function CreateAnimationModal(props: Props) { | |||
| `You selected too many meshes for the animation. Please keep the number of meshes below ${MAX_MESHES_PER_ANIMATION} to create an animation.`, | |||
| ); | |||
|
|
|||
| const validationStatus = hasEnoughMags && isDtypeSupported && isDataset3D && !isTooManyMeshes; | |||
| const isBoundingBoxEmpty = boundingBox.getVolume() === 0; | |||
There was a problem hiding this comment.
But here the BBox is not intersected with the color layer.
-> a color layer of bbox (0,0,0,10,10,10) and a bbox of (10,10,10,10,10,10) would also lead to zero volume. But this is not properly checked here.
There was a problem hiding this comment.
Or does such a scenario not result in an error?
I would at least say, that this would be an unwanted scenario, no part of the color layer would be rendered
There was a problem hiding this comment.
Good idea. I added that as an additional check in my latest commit.
MichaelBuessemeyer
pushed a commit
that referenced
this pull request
Jun 17, 2024
* validate BB size to be larger then 0 * changelog * appy PR feedback
aaronkanzer
pushed a commit
to lincbrain/webknossos
that referenced
this pull request
Jun 17, 2024
* validate BB size to be larger then 0 * changelog * appy PR feedback
MichaelBuessemeyer
added a commit
that referenced
this pull request
Aug 28, 2024
* Add dataset metadata input to details view in dashboard * update docs * Improve mobile support of views (#7876) * make login, registration and forgot password view more responsive to mobile devices - includes adding a link back to login for password reset view * make dashboard dataset view less awful * add changelog entry * Mapping in default view config (#7858) * allow viewing a mapping * WIP: enable setting default active mappings in default view configuration * add default active mapping to default view configuration - add config settings to dataset settings default view config tab - auto load initial default mapping * fix bottom margin of layer settings - was broken in view mode with multiple segmentation layers; the "Save View Config..." Button had no spacing to towards * fix having the mapping settings enabled for layers with default mapping * WIP: move default mapping store proeprty to dataset layer config - undo moving mapping selection to a shared component * moved default active mapping from separate select to view config layer config - Also added validation check whether the mapping exists in the dataset view config settings * allow saving no default active mapping from view mode & model init to new dataset config version * rename defaultMapping to mapping * cache available mappings fetched from backend when checking config in dataset settings * remove logging statements * clean up * add changelog entry * apply pr feedback * Fix animation modal color layer validation (#7882) * fix animation modal color layer validation * changelog * rename variable * apply pr feedback * improve error message of uint24 layers in animation modal --------- Co-authored-by: Michael Büßemeyer <michael.buessemeyer@student.hpi.de> * Validate animation job bounding box (#7883) * validate BB size to be larger then 0 * changelog * appy PR feedback * force serialization of dataset details prop in compact version of dataset * WIP adding details to folder * First version metadata support for folders * fix folder listing query * finish frontend support for folder metadata * default to undefined columnKey and not null to potentially prevent frontend dataset filtering - reason: ll.530 checks for sortedInfo.columnKey == null and columnKey is per default "" which makes this check fail and thus the chained map will never be executed * add evolution * update test db init data & folder snapshot test & bump schema version * remove unused import * also blur on property input * only support / show current error * improve css * WIP: use new json schema to save dataset & folder details and re-style metadata table * fix backend to work with new type of details dataset / folder field & WIP restyling of metadata / details table * WIP: improve dataset details table styling and add support for different types * details table for folders and datasets version 2 * WIP: adapt migration to new details format * uncomment ci tests * mini migration fix * - rename details to metadata * fix schema * remove unused backend imports * only update metadata set of metadata table when folder / dataset changes * ensure flushing updates on unmount of metadata table & increase debounce time * remove accidental change * WIP apply styling feedback & refactor handling metadataentry type * remove unused css * keep old dataset while updating & refetching in the dataset details view * remove unused code as search support for metadata entries is currently not planned * do not include metadata in dataset compact version * Fix Dataset refetching - also update datasetById when updating a dataset in the dashboard - Avoid spinner when updating metadata - Avoid unnecessary updates by guarding the debounced flush against not having pending calls to the debounced function * have fix width for metadata table cell contents to ensure consistent table column width and consistent layout - Ignore adding new metadata entry when there is already an empty one to avoid showing an error * enable selecting current folder of folder tree view as active details element * do not have initial empty metadata row * WIP: Apply styling update * Finish next version * hopefully fix flickering bug in when two different metadata updates have a cyclic race against each other * Only have default metadata on species, brainRegion & acquisition when creating a publication * also fill full metadata table width in case of an empty table * allow changing prop name although it is a duplicate; do not save metadata in case it has errors * clean up code for review * add changelog & migration entry; rename evolution * also rename revision; add comments to revision; remove dev logging * Apply PR Feedback * move metadata table to own file * only update when metadata changed & refactor code * fix updating the wrong dataset or folder with the newest metadata version of the table * allow multiple error rows & only update local metadata set if new folder/dataset or update failed - Try forcing dropdown menu to stay open during update -> does not work atm :/ * update preview image * fix color layer / segmentation layer switchero bug * remove periodic autosave an replace with explicit save via button or autosave when changing focused dataset / folder * re add auto saving mechanism and do not have nested components to avoid frequent remounting * update snapshots * refactor code & remove `useWillUnmount` which sent outdated metadata update to the backend breaking the whole metadata feature. * remove unnecessary dependency from useEffect accidentally added in a different pr * include metadata in full dataset update route * fix full update dataset route for metadata support * remove option to update tags (as they will no longer be rendered in the frontend) * remove index from initial metdata added to datasets with publication * apply pr feedback (testing pending) * lint frontend * do not send updates while a row of the metadata table is focused - Also: Use up-to-date metadata value on unmount effect (previously the inital value was sent to the server) * avoid lost isSaving updates due to changes to `focusedRow` state while updating. Due to changes to `focusedRow` while saving the previously used isMounted boolean was set to false when changed during the update post to the backend and thus the update to isSaving never got through the guarding. => Now it is explicitly tracked via a custom hook to avoid this scenario * fix sending double updates on unmount * apply pr feedback * migrate existing tags into metadata * remove unwanted additional wrapping array around migrated tags in metadata value entry * add undo migrating tags to metadata to revision * fix revision schema * give delete metadata entry button some more space; align key input to top & center delete button * update schema migration version * apply feedback * readd tag support in frontend (and backend) * fix backend * make metadata non nullable * update snapshots tests * fix metadatatype type definition * fix reversion against default null values of re-added details column --------- Co-authored-by: Tom Herold <tom.herold@scalableminds.com> Co-authored-by: Florian M <florian@scm.io> Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com> Co-authored-by: Michael Büßemeyer <MichaelBuessemeyer@users.noreply.github.com> Co-authored-by: Florian M <fm3@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds another validation for the animation modal. It checks that the user selected bounding box size is larger then 0.
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)