Fix/improve image loading #729
Conversation
…a-foundation#688) This is an improvement from the 2 previous attempts (commits) at fixing issue icosa-foundation#688 because it modifies the original logic way less. We just make ResetState() call RefreshDescription() every time. However, this version still has atleast one problem: - an image can have an error for other reasons too, not just for being too large. We're currently checking if the image has *any* error. (I think we need to create a way to check for image size error?)
…load error-specific message - Add a new entry to the ImageState enum for the error where an image is too large to load (ErrorImageTooLarge) - Only 2 error messages right now: a generic one "Image failed to load" and for images that are too large "Image too large to load" - Move ImageErrorExtraDescription() from LoadReferenceImageButton.cs into ReferenceImage.cs, because it needs access to the error states.
…ired when adding new items to enums
Not needed anymore as ResetState() sets the button unavailable if the image is invalid
… 'Image too large to load' will display
|
In the original issue #688 @andybak mentions:
I'd like some clarification on this. Some questions:
These are the current values for Quest: So ~10MB is the max file size and 4352*4352 is the max amount of pixels. I have tested with 16k res images on Quest 3 and I'm not crashing, and the memory limit OB displays increases only by like 1/50? |
[CI BUILD DEV] [CI BUILD]
Reverting to get back to the original limits for now. Still need to figure out if we can increase the limits.
I thinks so - they have quite different memory limits (Quest 1 especially)
It's a tough one and I'm not sure to be honest. It's one of the reasons I never started on this myself. I'd probably say we want to protect the user from getting into a state they can't easily get out of. Does undo resolve things if the image is so big it slows everything down? I suspect they would need to quit and restart. I know our sketch limits are based on very crude heuristics and is easy to fool. We also don't have autosave on the Quest which changes the risk equation somewhat.. I'd also say that I'm happy with any PR that improves the current status quo. We can always iterate. |
|
@andybak the current version is mergeable. It has:
|
Because the LoadImageCache(..) requires the fullpath, not just the name
This is a follow up from #722. The goal is to improve the image loading error functionality and figure out the correct size limits for Quest.
The first improvement makes sure that for images that have too large dimensions, the correct error message "Image too large to load" will be displayed in all cases. Previously, it was only working for images that have too large of a file size.