Implement Invisibility Marker

[rexy712]

I opened up #1264 a while back and I still want to implement invisibility markers. So this is my attempt.

I have no prior typescript or javascript experience outside of hacking on this project, so I'm sure this isn't following best practices. I did what I was capable of to get the minimal functionality I wanted so I could use it on my own PA instance.

Now I wanted to share the work (which felt like a lot but really doesn't look like it) and see if it can be made into something that is up to snuff to be included in the project. Or maybe inspire someone to make a proper version.

Commenting on the code, I made the IconManager class as a place to hold the image so that only one copy could be shared with all shapes. I couldn't find anywhere in the codebase that looked like the right place to do that, so I just made a super simple one. The reason I made a Map data structure to hold the images is because I had an idea to implement other possible icons that would draw on top of shapes, but I didn't want to get in too deep yet :)

As for actual functionality, the marker scales and translates properly to follow the shape it's attached to and always remains in the upper right of the bounding box. The toggle within the shape properties menu turns it on and off as expected.

This is what the invisibility marker looks like on basic tokens with a few different colored borders.

[Kruptein]

Hey, thanks for contributing this. You made this a draft PR, are there some adjustments still pending ?

The main postDraw logic makes sense with what you know. I'm working on some other ways to render icons, which I already started work on with the new note icon overlay (See icon shapes). There are still some small kinks to work out there so I'm not going to force you to adapt to that system at this point in time, but I might change it myself in the future.

The main downside of the postDraw approach which the newer system doesn't have is that async call needed to load the image. I'm not super fond of having an async call in the core render code. An alternative approach could be to just return an image or undefined if not loaded yet in the render code and ensure the loading is done in some other place. (e.g. in the main Game.vue init or when setting the showInvisibile property might be options)

I would also for sure move the icon manager code to a separate file in core/utils and it doesn't even have to be a class given that there should only ever be 1 instance and there isn't any other benefit to it being a class.

Now given that I will be changing some of this code in the near future (e.g. the IconManager will be kinda redundant), I would totally understand if you don't feel like wanting to bother with changing the things I mentioned. If you're not demotivated, feel free to modify it 😄

[rexy712]

First of all thanks for taking the time to give feedback! I really do want to help with what I can on this project, but unfortunately my experience in typescript and javascript are virtually zero.

You made this a draft PR, are there some adjustments still pending ?

The reason it's a draft is just because I felt like it'd be too forward to make it a real pull request with how crudely it's currently setup. I wasn't aware if you were working on this feature already or not, so I either wanted feedback on how to make it acceptable quality or be told you had already started implementing this.

I'm working on some other ways to render icons, which I already started work on with the new note icon overlay (See icon shapes).

My initial thought was using font awesome icons but I wasn't confident on how to do that. I completely missed the fontAwesomeIcon.ts file. Then I saw the eye slash icon already in the project and just decided to borrow it instead.

I would also for sure move the icon manager code to a separate file in core/utils and it doesn't even have to be a class given that there should only ever be 1 instance and there isn't any other benefit to it being a class.

Yeah moving it to its own file is something I should have done.

I took a look in fontAwesomIcon.ts and saw how you load images there. So you're suggesting to simplify to something like this?

function fetchEyeIcon(shape: IShape) : HTMLImageElement 
    const img = new Image();
    img.src = baseAdjust('/static/img/eye-slash-solid.svg');
    img.onload = () => {
        shape.invalidate(true);
    }
    return img;
}

I come from a C/C++ background, so I'm used to manually managing memory and preventing duplication of resources. Hence I made the IconManager to load the image once and reuse that same variable for any shape that requested it. I was worried not doing so would create a lot of duplicates of the same image. Is that not true in this situation because the browser tracks them for you or something?

Now given that I will be changing some of this code in the near future (e.g. the IconManager will be kinda redundant), I would totally understand if you don't feel like wanting to bother with changing the things I mentioned. If you're not demotivated, feel free to modify it 😄

I mean if you're already underway working on a better version, I'll drop this unless you wouldn't mind a stop-gap solution. I originally just did this as a (supposed to be) quick hack for myself. I ended up learning a decent amount about typescript in the process so whether I do more work on it or not, it's been worth my efforts.

[Kruptein]

And then do the async fetching elsewhere?

Yeah indeed,

My worry was that the image wouldn't be draw after the load finished

In theory this can indeed happen, in practice I would assume that there is at least some regular refresh of the screen causing the postDraw to run after the image has been loaded. But there could be a small period without the icon appearing yeah. This could be handled specifically by providing the shape id to the async loader and calling invalidate on the shape once the image is loaded.

My initial thought was using font awesome icons but I wasn't confident on how to do that. I completely missed the fontAwesomeIcon.ts file. Then I saw the eye slash icon already in the project and just decided to borrow it instead.

Hey that was a very sensible approach, the fontAwesomeIcon stuff is brand new, so chances were slim you were going to find it as I don't really have proper developer docs. Something I need to address at some point :D

I come from a C/C++ background, so I'm used to manually managing memory and preventing duplication of resources. Hence I made the IconManager to load the image once and reuse that same variable for any shape that requested it. I was worried not doing so would create a lot of duplicates of the same image. Is that not true in this situation because the browser tracks them for you or something?

No that was absolutely the right call and I would still hold a cache, something roughly like the following:

// utils/icon-cache.ts or something
const icons = new Map<string, HTMLImageElement>();

// We could have exported `icons` itself as well, but this prevents modification from outside this file
export function getIcon(name: string): HTMLImageElement | undefined {
    return icons.get(name);
}

// Depending on where this function will be called, passing the shape might be difficult
export async function loadIcon(name: string, url: string, shape: IShape): Promise<void> {
    if (icons.has(name)) {
        shape.invalidate(true); // depends on how the call will be used whether this is necessary.
    } else {
        const img = new Image();
        img.src = baseAdjust(url);
        img.onload = () => {
            shape.invalidate(true);
        }
        icons.set(name, img);
    }
}

I mean if you're already underway working on a better version, I'll drop this unless you wouldn't mind a stop-gap solution. I originally just did this as a (supposed to be) quick hack for myself. I ended up learning a decent amount about typescript in the process so whether I do more work on it or not, it's been worth my efforts.

I don't mind having something in between, my plans don't always end up coming to fruition in the short term 😅. I don't mind either if you're just going to stick to your private patch and wait for me to get my work done, so I leave it up to you.