annotorious / annotorious-openseadragon

An OpenSeadragon plugin for annotating high-res zoomable images
https://annotorious.github.io
BSD 3-Clause "New" or "Revised" License
121 stars 43 forks source link

Annotating an image creates an annotation on all previously viewed images #177

Closed RonyKordahi closed 1 year ago

RonyKordahi commented 1 year ago

I've successfully implemented OSD with the annotorious plugin in React following these two guides:

Everything works as intended, but the problem I'm facing is when I view 4 different pictures and then decide to annotate only the last one, annotorious is actually creating the annotation on all 4 pictures.

As far as I can tell, the .on event is accessing some kind of memory? Honestly I don't even know. What I do know is it fires once for every image I've opened in the viewer before the current image I'm annotating, and it's able to access the unique ID and URL of each of those past images.

I've been stuck on this for several days now and I don't know what to do. Any suggestions would be welcome!

DiegoPino commented 1 year ago

Hi, Annotorious has an internal, global, Annotation storage. I believe (have not read the guides, sorry) It is up to you to clear/load new ones and keep them in an intermediate storage on any OSD page change.

What we do is at the Viewer (OSD) level is to add a page handler e.g viewer.addHandler("page", function (data) {....

That handler should:

In other words, to which image/picture/page an annotation is tied is up to you, but if you don't stash, clear, reload on each Page change then Annotorious will keep feeding the same annotation storage.

RonyKordahi commented 1 year ago

I figured it would have some kind of internal storage, but I couldn't find anything about it in the guides so I was very much at a loss for what to do.

The way I currently have it set up is the same as the guides on Medium I linked in the first post, everything is being done through localStorage. The annotations are being stored in localStorage under a specific ID associated with every image. It will be changed later but I wanted to figure out the source of this problem first.

Getting all current annotations for that specific image and setting them after a reload is working fine. Saving them for that particular image also works well.

The real issue I'm having is the annotorious.on("createAnnotation", (annotation) => {...}) event is firing once for every image I've previously viewed in OSD before annotating the current image I've selected. If I look at image1, image2, image3, and image4, but only annotate image4, the event still creates an annotation for all 3 previous images even though there's no annotations in memory (both localStorage and the annotations array). It seems to loop on something that I don't know how to access and clear.

rsimon commented 1 year ago

Are you creating separate instances of Annotorious for every image? And are destroying the instances when you upload the images?

Or are displaying multiple OSD images in parallel?

RonyKordahi commented 1 year ago

I'm only displaying one image at a time. I'm passing down the OSD image as a prop from a parent component to a child component, which is then rendered in the OSD viewer.

There's only one instance of Annotorious and OSD being created in the child component. I've tried closing the OSD viewer with OpenSeaDragon.close() but that hasn't helped much.

rsimon commented 1 year ago

Can you post the code you use to initialize?

RonyKordahi commented 1 year ago
    useEffect(() => {
        viewer && viewer.destroy();

        const initViewer = OpenSeaDragon({
            id: "openSeaDragon",
            prefixUrl: "openseadragon-images/",
            animationTime: 0.5,
            blendTime: 0.1,
            constrainDuringPan: true,
            maxZoomPixelRatio: 2,
            minZoomLevel: 1,
            visibilityRatio: 1,
            zoomPerScroll: 2,
            tileSources: {
                type: 'image',
                url:  'https://picsum.photos/1000',
                // buildPyramid: false
            }
        })

        setViewer(initViewer);
        setAnno(Annotorious(initViewer))

        return () => {
            viewer && viewer.destroy();
        };
    }, []);

The useEffect is executed on component render. The anno state variable holds the Annotorious instance, and the viewer state variable holds the OSD instance.

There is also the following function which is executed every time a different image is selected:

    const initAnnotations = () => {
        const storedAnnotations = getLocalAnnotations();

        if (storedAnnotations) {
            const foundAnnotation = JSON.parse(storedAnnotations);
            setAnnotationsList(foundAnnotation);
            anno.setAnnotations(foundAnnotation);
        }
        else {
            // cleanup so annotations don't persist through images
            anno.setAnnotations([]);
        }

        anno.on('createAnnotation', (annotation) => {
            const newAnnotations = [...annotationsList, annotation];
            setAnnotationsList(newAnnotations);
            setLocalAnnotation(newAnnotations);
        });

        anno.on('updateAnnotation', (annotation, previous) => {
            const newAnnotations = annotationsList.map(val => {
                if (val.id === annotation.id) return annotation
                return val
            })
            setAnnotationsList(newAnnotations)
            setLocalAnnotation(newAnnotations)
        });

        // NEEDS WORK
        anno.on('deleteAnnotation', (annotation) => {
            const newAnnotations = annotationsList.filter(val => val.id !== annotation.id)
            setAnnotationsList(newAnnotations);
            setLocalAnnotation(newAnnotations);
        });
    }
rsimon commented 1 year ago

Ok thanks. There is only one Annotorious instance, true. But you're adding a new set of event handlers whenever a new image loads. Therefore: you're adding four event handlers, you'll get four event responses :-)

RonyKordahi commented 1 year ago

I've tried only adding the event handlers on creation of the instance, but unfortunately none of them fire. I'll try again maybe my method was flawed the first attempt.

EDIT: fixed typo

RonyKordahi commented 1 year ago

Nope sorry I was wrong, it does fire but for some reason is no longer able to recognize the image I'm passing down as a prop as a valid object.

There's a mistake in my OSD initiating code up above. The tileSources is only there as a test.

The real image is a slide being fetched from "https://openslide-demo.s3.dualstack.us-east-1.amazonaws.com/info.json". I'm passing down the slide information as a prop from the parent component to the child component that renders the OSD and Annotorious instances. If I initiate the events 4 times, each initiated event holds one image in memory. If I initiate them only once, they are unable to recognize the image as a valid object and it is forever seen as null across all the events.

EDIT: typo

rsimon commented 1 year ago

Hard to tell without seeing the full code. But if you're passing in things as props and not listing those props in the effect's dependency array you might have the initial values as closure. Easiest way is probably to re-create Annotorious when you recreate the viewer and/or load the image. .destroy() will properly destroy anno & you can create a new instance.

RonyKordahi commented 1 year ago

I was thinking the exact same thing. It is passed in the dependency array but I think the events are victims of closure. I'll play around with it some more and post any findings I get.

RonyKordahi commented 1 year ago

Yep it absolutely was a closure issue after all. And the multiple instances were the cause of the previous images. I've never seen closure behave that way, really threw me for a loop. I solved it by removing the event subscription from the function in my post above and putting them in their own useEffect that handles unsubscribing as well:

    useEffect(() => {
        if (anno) {
            anno.on('createAnnotation', createAnnotation);

            anno.on('updateAnnotation', updateAnnotation);

            anno.on('deleteAnnotation', deleteAnnotation);
        }

        return () => {
            if (anno) {
                anno.off('createAnnotation', createAnnotation);

                anno.off('updateAnnotation', updateAnnotation);

                anno.off('deleteAnnotation', deleteAnnotation);
            }
        }
    }, [image, anno])

Doing Annotorious.destroy() didn't work out for me because state updates are asynchronous but can't be waited on unfortunately.

EDIT: more typo 🤦

rsimon commented 1 year ago

No worries, I've had some weird closure issues with React myself a few times... Yes, actually the .on / .off solution is probably the cleanest to begin with!

Interesting thought that .destroy() didn't work. It's supposed to behave pretty much the same way as .on, .off. (Did you call it on the anno instance? I.e. anno.destroy() not Annotorious.destroy()?) Anyways: good to see it works :-)

RonyKordahi commented 1 year ago

I did but re-initializing the state to a new Annotorious instance is slower than changing the image so it threw me an error. I didn't try to handle it in a cleaner way though so it's likely that my implementation of it was a bit flawed.