Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loadout Items ADR #1680

Closed
wants to merge 3 commits into from
Closed

Loadout Items ADR #1680

wants to merge 3 commits into from

Conversation

halgari
Copy link
Collaborator

@halgari halgari commented Jun 24, 2024

Pretty simple stuff, just documenting the plans we've had for awhile about loadout items (#1336).

@erri120 erri120 requested review from erri120 and Al12rs June 24, 2024 16:06
Comment on lines +117 to +120
The primary motivation for this change is to move questions of what a specific object is from code-time to runtime. Displaying
items in a grid or a tree, or projecting parts of the loadout item tree into a list all now become runtime concerns and
we are free to project the data in any way required. New file or container types can be added to this process and existing
grids and lists should work as-is.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section can be expanded to include one of the main aspects of this: optimized for reading. The goal is to easily grab any data in the correct view. The SMAPI diagnostic emitters are a great example, most of them don't care about anything but SMAPI manifest files and SMAPI mods.

Copy link
Member

@erri120 erri120 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should clearly define some models in this ADR. Specifically, what you refer to as ContainerItem. This model is going to replace what we currently refer to as a "Mod" and I want us to get it right, as it's going to be the centerpiece for almost everything else.

I'm not married to the term ContainerItem yet, but for now, here's a shortlist of things we need to somehow deal with (this is not a complete list):

  • Game Files
  • "Override"
  • Nexus Mods Collections
  • SMAPI Mods, where each mod has its own manifest file. One archive might contain three SMAPI Mods, even though they came from the "remote mod" (using this generic term to represent a Nexus Mods mod page, open to change)
  • SMAPI config files

One pain point I want to avoid with Loadout Items is figuring out what goes into the beginner-friendly Loadout grid. Sure, we can have some expert view that shows every single Loadout Item, but design wants a simple view that "shows mods", which is why the entry on the left menu says "Mods" and not "Loadout Grid".

As mentioned many times before, the term "Mod" is overloaded, and we need to find something else. I want us to find that term, and create a clear definition with this ADR.

Comment on lines +89 to +92
The Synchronizer will need to be updated to handle the new model, flattening a loadout becomes a matter of finding all `File`
items that point to the loadout, then walking all those items to make sure each parent is enabled. Stated another way, the
loadout is flatted down by grouping all loadout items by `To` that are not an ancestor of a item that has the `Disabled` marker
attribute.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bottom up approach, while more intuitively easy to parallelize, would require performing a walk up the tree for every single file to check if all parents are enabled.

Would it not be more efficient to walk down the tree of only the branches that are enabled?
Skipping, in this manner, large chunks of files as top level items exclude them.

@halgari halgari closed this Jul 17, 2024
@erri120 erri120 deleted the loadout-item-adr branch July 17, 2024 08:43
@Sewer56 Sewer56 mentioned this pull request Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants