DIP17: Move images into the plugin repo

[philpax]

With the current implementation of DIP17, images are currently stored alongside the manifest. I think this doesn't really make sense if we're pulling the repo at a given revision anyway; we might as well pull the images from the repo.

This should make the submission process even more smooth, especially once SamplePlugin is updated for it.

Might need to add some words to make it explicit that the images are coming from the repo, if it's not obvious enough already.

[philpax]

stakeholder feedback ping: @goatcorp/dalamud @goatcorp/plugin-approval @Styr1x @KazWolfe @NotNite

if no concerns, this will be merged on Monday and we'll try to get people moved over before 6.2, but we'll support images/ in DalamudPlugins for a couple of weeks still

[KazWolfe]

Is the plan to have plugin images held within PluginDistD17, or just as an IconUrl link to the base repository?

If the former, I'd like to recommend that images/ remain sourced by default from the DalamudPackager output directory (so, in the same folder as latest.zip) rather than using the repository root. A nonzero number of plugins already implement this behavior through DalamudPackager's automatic copying of images from the appropriate OutputPath (see DalamudPackager.cs#L182).

As far as I can tell, the system was designed this way to allow developers to simply copy-paste the generated release folder into their local DalamudPlugins and PR it off, without having to manually find or deal with assets. Most plugins that use this system have additional instructions in their .csproj files to handle copying images to their output paths accordingly. Reusing this folder would add one less change that needs to be made to existing build processes, and allow developers more freedom in icon naming and positioning in their project.

As an example, the following stanza will become impossible under the proposed solution:

<ItemGroup>
    <Content Include="../assets/icon.png">
        <Link>images/icon.png</Link>
        <CopyToOutputDirectory>Always</CopyToOutputDirectory>
    </Content>
    <Content Include="../assets/marketing/image*.png">
        <Link>images/%(Filename)%(Extension)</Link>
        <CopyToOutputDirectory>Always</CopyToOutputDirectory>
    </Content>
</ItemGroup>

While the validity of using such a stanza can certainly be questioned, it still structures things appropriately in the final output in such a way that DalamudPackager is happy with and would have produced a valid (and image-loaded) artifact.

[philpax]

Seems like a reasonable change, can you propose a change to the DIP text to explain what the behaviour would be?

[Aireil]

I do not think I get Kaz's suggestion, you are no longer PRing the output of DalamudPackager, so why would it matter if images are copied over or not?

Is the plan to have plugin images held within PluginDistD17, or just as an IconUrl link to the base repository?
If the former....

Isn't it the latter? Well, not exactly, as it will be relative to the base repository: cf proposed change.

[KazWolfe]

In response to @philpax, I'd propose something along the lines of the following for the template file.

# Optional: Where the icon and marketing images for this plugin are stored, relative to the root
# of the repository. If empty or unspecified, the builder shall use the `images` folder present
# in the DalamudPackager artifact directory.

Additional documentation reflecting the following point should also be added.

The build environment shall copy icon.png, and image[1-5].png from the defined image_path in the plugin manifest to the images/ folder in the final output directory for the plugin's build process. If image_path is unset, the build environment shall instead use the images/ folder present in the DalamudPackager artifact directory for that plugin's build.

I would lastly propose the following additional constraints:

The build shall fail if:

• image_path is unset and an images/ folder does not appear in the DalamudPackager output path
• No icon.png file is present in the used image path.

---

I will note (in response to @Aireil) that currently image loading behavior seems completely undefined. From a read of the DIP (especially now that IconUrl is being removed from the manifest), it is not clear where images are being loaded from or stored. Some plugins have images stored alongside their manifests, suggesting that the intended implementation is that images are stored on and downloaded from Dalamud-controlled systems rather than reading a URL to a possibly-uncontrolled source. I think that copying images overall is a net benefit, as:

• It prevents a rogue developer from swapping out an icon or marketing image to a file not approved by the Plugin Review Team.
• It prevents game clients from sending traffic to Git servers that (a) may not necessarily be able to handle the load of clients, (b) may be logging or tracking IP addresses, or (c) do not necessarily accept or want HTTP connections for non-Git purposes.

If this is indeed the desired outcome, I believe it additionally makes sense to officially define that the icon and marketing images for plugins be downloaded from the images/ directory, in the same place as the latest.zip binary blob. (Editor's note: it may probably make sense to define all of the plugin download system; DIP17 seems completely silent about what happens after build right now).

If this is actually implemented, we circle back to Aireil's second point: we're already going to be copying the images from the source repository. We can either allow the user to define a fixed path for where these images will live, or default to reading from the images/ folder in the DalamudPackager output. I would prefer reusing the already-existing DalamudPackager option as it is already in use by quite a few plugins, and allows plugin developers more freedom in structuring their repository in a way that makes sense to their projects and workflows.

[Aireil]

OK I get it now, I misunderstood what you suggested.
Concerning your note, when I said copying, I was thinking of a local copy on the plugin dev computer, not on the build system.
Images would, of course, be copied from the repo and not be directly loaded from a random URL.

Suggestion itself sounds good.

[Styr1x]

I think the change is reasonable. I' not sure about @KazWolfe proposed additional change to use the packagers output as a fallback. Flexibility adds complexity and in case of D17 developers already have to modify their projects, thus its also a good chance for unifying the approach of how things are done. This will also make things easier in the long run.

How about only allowing one specific way of setting the images?

[KazWolfe]

I will admit freely that part of my own reasoning for requesting the change to continue using Packager is because I have a bit of an unusual configuration in XIVDeck. Due to requirements with Stream Deck plugin packaging, I opted to place my icon file as [email protected] in the Stream Deck Plugin assets folder. My Dalamud plugin's csproj files pull this file from the appropriate place at build time and include it in my build's output.

Updating this process is a moderate pain, and I'd rather not have to redo my assets structure, my Dalamud plugin configuration, and my Stream Deck packaging unless absolutely necessary. Any such solution would just involve creating duplicate versions of images (not great), doing a lot more file moves and renames at build time (also not great), or having assets spread all over my project tree and then moved (also also not great). There's already enough complexity dealing with one system wanting assets in a specific place and structure; having to comply with two systems that need things structured a certain way will be extra (and unnecessary) pain.

If we have to have a single specific way, my vote is still in favor of sticking with existing DalamudPackager compatibility. A lot of plugins are already set up for it, it allows devs that need more complex processes at build times to do what they need, and I don't really think it adds that much extra pain. The builder is already copying latest.zip from the exact same directory that should hold the images/ directory, after all. I understand that one (barely used) plugin does not tip the scales, but allowing devs to have some degree of latitude goes a long way for those that actively benefit from it.

[Styr1x]

I can live with DalamudPackager being the source of truth for the images, as its already part of the build process. This would also add all the flexibility one might need, including the post processing of images inside the build (like creating different sized images from an source image).

[KazWolfe]

Another option comes to mind as well: allow developers to specify individual files rather than just a folder. This reduces build complexity by no longer requiring any changes to be made to the csproj, allows DalamudPackager to do one less thing, and still gives developers control over what is pulled from where. Instead of using an image_path, two new fields would be used: icon_path, and marketing_images:

# A path to the icon, relative to the repository root.
icon_path = "/assets/images/[email protected]"

# Optional: Allows specifying screenshots or other marketing images to be included in the
# plugin listing. If not included, marketing images will not be used for this plugin.
marketing_images = [
    "/assets/images/marketing/hero.png",
    "/assets/images/marketing/screenshot1.png",
    "/assets/images/marketing/screenshot2.png"
]

---

All of the above may also actually be served better as values inside the proposed <DalamudPlugin> stanza of the csproj, thereby keeping like material (punchline, description, icon, and marketing images all feel like listing-related material) together, which is also a benefit.

If modifying DalamudPackager is in scope, DalamudPackager can read from <IconPath> and <MarketingImage> and handle copying them to the appropriate images/ folder inside the artifact path. The build system will then just copy images/ alongside the latest.zip. Everything is kept in one location, repo structures remain fully flexible, and no <Content> or similar directives need to be written by developers.

<ProjectExtensions>
    <DalamudPlugin>
        ... 
        <Name>My Awesome Plugin</Name>
        <Punchline>It's a plugin. And it's awesome.</Punchline>
        <Description>This plugin is super ultra cool and awesome and it has like a million lasers and it drives a super fast car and everyone likes it and it's awesome and stuff.</Description>
        <IconPath>../assets/images/[email protected]</IconPath>
        <MarketingImage>../assets/images/marketing/hero.png</MarketingImage>
        <MarketingImage>../assets/images/marketing/screenshot1.png</MarketingImage>
        <MarketingImage>../assets/images/marketing/screenshot2.png</MarketingImage>
        ...
    </DalamudPlugin>
</ProjectExtensions>

[philpax]

I'm happy with using DalamudPackager as the only source of truth; that makes things simpler, especially as users already have to use it for DIP17 compliance. The only change I'd make to your proposal is to use a hierarchy:

<ProjectExtensions>
    <DalamudPlugin>
        ... 
        <Name>My Awesome Plugin</Name>
        <Punchline>It's a plugin. And it's awesome.</Punchline>
        <Description>This plugin is super ultra cool and awesome and it has like a million lasers and it drives a super fast car and everyone likes it and it's awesome and stuff.</Description>
        <Images>
            <Icon>../assets/images/[email protected]</Icon>
            <Marketing>../assets/images/marketing/hero.png</Marketing>
            <Marketing>../assets/images/marketing/screenshot1.png</Marketing>
            <Marketing>../assets/images/marketing/screenshot2.png</Marketing>
        </Images>
        ...
    </DalamudPlugin>
</ProjectExtensions>

Are there any objections to

1. moving the definition of where to find images entirely into DalamudPackager, and not supplying it anywhere else
2. having PluginDist extract the images, so that it is always representative of what's being shown to the user?

edit: this is unlikely to be ready by Monday, but that's fine. We'll encourage the use of this and disable images/ next time around.

[Styr1x]

I'm fine with @philpax proposal; will add it to Plogon's image handling (aka copy images from packages out) once this PR is merged.

[philpax]

Okay, I've made the proposed change. Can everyone weigh in, and if there's no objections, we'll merge it in a week's time and start implementing?

[philpax]

Last call, one day left!

[philpax]

it is done