Add capability to use images instead of text for SRL v5 and SRL v8 generators

[rpierce99]

There are some advantages to using images for some or all goals, and this PR makes it an option of the user if they'd like to use them or not. Obviously the community would be responsible for providing images for the goals if they wanted to have the option. Below is a screenshot showing what the images look like and what the enable/disable option looks like.

Let me know if there's anything I can do to make this more mergeable. Thanks!

[kbuzsaki]

Hi there, this looks great, thanks for the pull request! The code overall looks good and I'd be happy to merge it :)

There's one design concern that I have, which is that I'd prefer that the images be served from bingosync and not from external URLs. That lets me ensure that the files are always served correctly and that the contents aren't replaced maliciously. I was thinking something more along the lines of users submitting "image packs" that are versioned alongside the generators, with some ID scheme (possibly just the filename) to connect them in the goal list.

Thoughts on that approach?

[rpierce99]

There's one design concern that I have, which is that I'd prefer that the images be served from bingosync and not from external URLs. That lets me ensure that the files are always served correctly and that the contents aren't replaced maliciously. I was thinking something more along the lines of users submitting "image packs" that are versioned alongside the generators, with some ID scheme (possibly just the filename) to connect them in the goal list.

Thoughts on that approach?

i thought about designing it that way but you described the setup as single server without cdn so I was a little concerned about server load. To mitigate the URL concern we could only accept image URLs from services with known limits on URL reuse. I assume imgur and the line won’t let you replace an image but I admit I don’t know for sure.

if you’d rather have the images as part of the checked in content I’d be happy to take a stab at that.

[rpierce99]

Looks like Imgur was a bad example, their ToS wouldn't be likely to allow for this sort of usage. There are a couple of smaller players that don't prohibit this sort of usage, at least as far as I can tell, but it may be risky to rely on them.

[rpierce99]

@kbuzsaki I thought some more about this and I think the answer to your question is that I'd recommend supporting both. This PR would stay as is, allowing remote hosting when it makes sense, and through the magic of relative paths would support a 2nd icon pack feature that would be served off the same host, so once an icon pack is installed you'd put a relative reference like "/icons/zeldabotw/8towers.png" into the image property of the generator js line.

The still-to-be-built (and frankly still-to-be-designed) part of this is the code that would either

• Serve files directly from icon packs (perhaps just tar files to prevent zip bombs etc?) at the cost of that seek operation for every request but saving the complexity added by dealing with disk vs icon pack differences
• or move icon files onto disk when necessary so the static file portion could serve it up, speeding up the file serving but introducing the problem of keeping the disk cache in sync with the icon pack when it changes.

Let me know what you think about the plan.

[rpierce99]

I've updated this PR with a somewhat naive implementation of the image server, would love feedback on it. I'd still probably like to see caching/CDN implemented here, but since it's currently only a single file for a single game I don't think it will be an immediate concern.

Let me know what else you'd like to see before a merge, thanks!

[kbuzsaki]

I think that the images should be committed to directories in bingosync-app/static/... as individual files, not as zip/tarball archives. Then I'll have nginx serve them directly rather than routing through django. There's even a somewhat straightforward upgrade path to a traditional CDN if necessary. People can send me the packs as zips/etc, but I'll unzip them before committing.

I'd also prefer if the goal lists didn't have to embed the full relative URL (e g. with a leading /static/foo/) since that's kind of an implementation detail that I might want to change and it's more toil for users. I think the image in the goal list should just be something like the name of the file, and bingosync should handle constructing the qualified url based on that.

So for example, the goal list might just have image: "8Towers.png", and bingosync would convert that to something like /static/bingosync/goal_images/zelda_botw_english/8Towers.png.

I'm undecided on what the precise URL structure should be (e.g. whether to use the numeric enum value rather than the name), but that's close enough.

[rpierce99]

Thanks for the comments, I can make these changes but I'm not sure I can test them, have you published your nginx config anywhere? We'd basically have to make nginx setup part of the development prerequisites to be able to run locally.

[kbuzsaki]

I believe django will serve the static files itself when run in development mode. That's how, for example, the css and javascript are being served.

See also https://docs.djangoproject.com/en/3.1/howto/static-files/

[kbuzsaki]

Taking a peek at the latest commit, let's please keep path information out of the generator code if at all possible. I'd like that to be invisible to users, and some users provide a completely custom generator script.

If the goal is to avoid duplicating icons for similar generators, my hope is we can accomplish that with symlink directories? It looks like nginx can be configured to do that?

[rpierce99]

Yeah, the goal there was to not have to upload the same images multiple times for the long, short, etc generators and it wasn't immediately obvious to me where I could find a string that was common to all of them. game_name appears several places, and I started writing it to use that, but game_name includes the variant as well. I thought about linking them at the file system but since git wouldn't represent it well (the path of the symlink on the committers machine won't match yours) and I didn't assume you'd want that much manual manipulation I went a different direction. I tried to keep the implementation details to a minimum and just include the single folder name, but if you'd prefer the symlink solution I can convert it over, or if you have another place in the code where I can pull a common name from I could use that instead. Let me know what you'd like to see. Thanks!

[kbuzsaki]

Can we use a relative symlink to avoid the issues with fully qualified paths?

As for what directory names to use for the deduped files, I think that's somewhat arbitrary. We could have one of the variants hold the images and have the others be symlinks to it, similar to how a game group has a "top level" variant that represents its place in the game list. Or we could have an internal directory to hold the deduped packs that has a generic name like botw and then make all game directories be symlinks to that one. It doesn't even need to be consistent across games :)

[rpierce99]

I think this version matches what you're describing, let me know, and thanks for the back and forth, appreciate the time.

[rpierce99]

I accidentally pushed additional work into this branch, working on pulling the counters stuff out into its own PR, sorry for the mixup
EDIT: all fixed up