Skip to content

Conversation

@halgari
Copy link
Collaborator

@halgari halgari commented Oct 8, 2025

This implements plugin sorting for SkyrimSE/FO4. But right now tests don't pass as we need a way to efficiently test against game files and have a valid game hash database

- Added SQL macros for plugin load order management (`load_order_plugin_files` and `plugin_sort_order`).
- Implemented `PluginLoadOrderVariety` for sorting and managing plugins.
- Extended `SkyrimSE` with sort order support via `ISortOrderVariety`.
- Introduced models and attributes (`PluginSortEntry`, `PluginReactiveSortItem`, `ModKeyAttribute`) to enhance plugin data representation.
- Updated `NexusMods.MnemonicDB` packages to version `0.28.1` for compatibility.
- Adjusted project files to include SQL resources and align with the new load order functionality.
- Replaced unused and commented-out code in `PluginsFile.Write` with a simplified implementation based on database queries to generate sorted plugin lists.
- Refined SQL macros and added ordering in `plugin_sort_order`.
- Updated `PluginLoadOrderVariety` to improve item sorting and persist logic, including ordering and deletion checks.
…ntrinsic file path handling. This was causing a bug where disabled files would never show up as disabled in these queries
…plementation to ensure compatibility with future file scanner changes.
…sort logic

- Introduced `PluginLoadOrderIntegrationTests` to validate plugin sorting and load order management behaviors.
- Updated `plugin_sort_order` SQL to include explicit `ORDER BY SortIndex` for consistent results.
- Extended test framework with additional dependencies for plugin file handling.
@github-project-automation github-project-automation bot moved this to Development review in App Oct 8, 2025
@halgari halgari marked this pull request as draft October 8, 2025 15:21
@Al12rs Al12rs changed the title Creation loadorder Creation games loadorder Oct 9, 2025
{
private const string Namespace = "NexusMods.Games.CreationEngine.PluginSortEntry";

public static readonly ModKeyAttribute ModKey = new(Namespace, nameof(ModKey));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a comment to clarify what ModKey refers to in this context, as it could easily be equivocated to mean an Id for the LoadoutItemGroup mod, rather than an Id for the plugin.

I'm considering whether to use different names and aliases on top of the Mutagen ModKey, to avoid equivocation since in our code domain Mod means something different.


namespace NexusMods.Games.CreationEngine.LoadOrder;

public class PluginLoadOrderVariety : ASortOrderVariety<
Copy link
Contributor

@Al12rs Al12rs Oct 9, 2025

Choose a reason for hiding this comment

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

Should be named PluginSortOrderVariety for consistency

Suggested change
public class PluginLoadOrderVariety : ASortOrderVariety<
public class PluginSortOrderVariety : ASortOrderVariety<

Comment on lines +42 to +62
public override async ValueTask<SortOrderId> GetOrCreateSortOrderFor(LoadoutId loadoutId, OneOf<LoadoutId, CollectionGroupId> parentEntity, CancellationToken token = default)
{
var optionalSortOrderId = GetSortOrderIdFor(parentEntity);
if (optionalSortOrderId.HasValue)
return optionalSortOrderId.Value;

token.ThrowIfCancellationRequested();

using var ts = Connection.BeginTransaction();
var newSortOrder = new SortOrder.New(ts)
{
LoadoutId = loadoutId,
ParentEntity = parentEntity,
SortOrderTypeId = SortOrderVarietyId.Value,
};

var commitResult = await ts.Commit();

var sortOrder = commitResult.Remap(newSortOrder);
return sortOrder;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this into ASortOrderVariety directly.

Only reason RedMods have an override is that they have the extra RedModSortOrder model from previous iteration of the system. It's superfluous, but it was simpler to keep it rather than do a migration to remove it.

Comment on lines +67 to +70
$"""
SELECT ModKey, GroupName, GroupId, SortIndex
FROM creation_engine.plugin_sort_order({Connection}) WHERE SortOrderId = {sortOrderId.Value}
""")
Copy link
Contributor

Choose a reason for hiding this comment

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

As final cleanup, I moved all the queries out from the SortOrderVariety file into a separate file (RedModExtensions, but maybe PluginQueries.cs would be better here).

I think it makes it easier to find them and update them if we change something with the query system (e.g. we want to use something like Dapper to generate the SQL or some other change).

In general I like to keep the implementation details of how the data is accessed in the DB separate from the logic.

Comment on lines +247 to +249
// Insert new items at the start, sorted by newest creation order (ModGroupId)
// For cyberpunk RedMods, lower index wins, so we add new items at the start
results.InsertRange(0, itemsToAdd);
Copy link
Contributor

Choose a reason for hiding this comment

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

New items should be added at the end of the sort order for Bethesda games, rather than at the start.

We want newly added items to win by default, and for bethesda games we need to put them at the end of the order.

file.Size,
file.ItemType,
file.Id,
regexp_extract(file.Path.Path, '([^/]+)$') ModKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you interested in the entire path or just the filename? I think you might need to specify to capture group 1 of the pattern if just looking for the filename, as it should default to capturing the entire string otherwise.

Suggested change
regexp_extract(file.Path.Path, '([^/]+)$') ModKey,
regexp_extract(file.Path.Path, '([^/]+)$', 1) ModKey,

regexp_extract(file.Path.Path, '([^/]+)$') ModKey,
grp.Id GroupId,
grp.Name GroupName
FROM synchronizer.WinningFiles(db) file
Copy link
Contributor

@Al12rs Al12rs Oct 9, 2025

Choose a reason for hiding this comment

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

Only querying the winning plugins here is problematic because it is excluding all the plugins in disabled mods and collections.

Including plugins from disabled mods in the sorting has been an explicit design decision.
This was the solution Design came to when considering the issue of users being able to Toggle collections on and off.

If we don't include disabled items in the sorting, users would immediately lose all their sorting information upon disabling a collection.

Enabling the collection again would result in all the items getting sorted in random or alphabetical order.

Similarly, users don't want to have to reposition a plugin in the load order after having toggled the parent off and on again.

I would agree that users don't necessarily expect plugins from disabled mods to show up in the sorting view, but not including them here at the data layer would result in a very bad user experience.

MO2 has a system in place to remember the position of recently disabled items in the load order. I would need check again how the implementation worked (my impression was that it was more of a hack), but it is there to address the case of users toggling mods on and off.

I would start by including disabled items in the sorting, and I would instead look at options later to potentially filter out disabled items in the view proposed to users if we have an issue with too many plugins being shown.

Comment on lines +76 to +87
if (!x.ModId.HasValue)
return model;

model.ModGroupId = Optional<LoadoutItemGroupId>.Create(x.ModId!.Value);
var loadoutData = new SortItemLoadoutData<SortItemKey<ModKey>>(
model.Key,
true,
x.GroupName,
LoadoutItemGroupId.From(x.ModId!.Value));

model.LoadoutData = loadoutData;
return model;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a bit of an example of what I mentioned the other day regarding GameFiles not being Loadout Files.

It would be preferrable to be able to reference them in a homogeneous way like we do with other items in the loadout, either as LoadoutItems themselves, or by using some abstraction over LoadoutItems (which sounds less doable).

Not much to do for this PR specifically, but more of a note for how we want to handle game files in the future.

Comment on lines +17 to +18
WHERE file.Path.Location = nma_fnv1a_hash_short('Game')
AND file.Path.Path SIMILAR TO '(?i)^Data/[^/]+\.(esp|esl|esm)$';
Copy link
Contributor

Choose a reason for hiding this comment

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

The rows of this query should be deterministically sorted.

Ideally items that have been added most recently to the loadout should appear at the bottom, in case of parity I would go by plugin alphabetical order.

We want to ensure that new entries appear at the bottom of the order.

Comment on lines +15 to +16
LEFT JOIN mdb_LoadoutItem() itm on itm.Id = file.Id
LEFT JOIN mdb_LoadoutITemGroup() grp on grp.Id = itm.Parent
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to pass in the db parameter to mdb_LoadoutItem() and mdb_LoadoutITemGroup(), or we might be fetching from the most recent DB rather than the one we are interested in.

Comment on lines +21 to +25
CREATE MACRO creation_engine.plugin_sort_order(db) AS TABLE
SELECT sortOrder.Id SortOrderId, items.ModKey, sortItem.SortIndex, items.GroupId, items.GroupName
FROM MDB_SORTORDER() sortOrder
INNER JOIN creation_engine.load_order_plugin_files(null) items ON items.Loadout = sortOrder.Loadout
INNER JOIN MDB_PLUGINSORTENTRY() sortItem ON sortItem.ParentSortOrder = sortOrder.Id AND sortItem.ModKey = items.ModKey
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is used when writing Plugins.txt during synchronizer step, it needs to access the correct Db revision.

So MDB_SORTORDER(), MDB_PLUGINSORTENTRY() should take the db parameter as well I think.

Not sure what the null is doing in this case creation_engine.load_order_plugin_files(null), I would have expected that to also need to have the db parameter passed to it.

halgari added 14 commits October 9, 2025 07:14
…st framework with new test projects for SkyrimSE. Update `FileHashesService` for in-memory file system compatibility and adjust NexusMods library package versions.
…image error handling, and enhance SkyrimSE path resolution with `DataPath`.
…`.nx` archives and update integration test constructor visibility.
…`PackManifestToNx` helpers to simplify logic and reduce code duplication.
… error handling for missing environment variable, and add developer documentation for running integration tests.
@github-actions
Copy link
Contributor

This PR doesn't conflict with main anymore. It can be merged after all status checks have passed and it has been reviewed.

@github-actions github-actions bot removed the status-needs-rebase Set by CI do not remove label Oct 22, 2025
@halgari halgari marked this pull request as ready for review October 22, 2025 02:40
@github-actions
Copy link
Contributor

This PR conflicts with main. You need to rebase the PR before it can be merged.

@github-actions github-actions bot added the status-needs-rebase Set by CI do not remove label Oct 22, 2025
throw new NotSupportedException();
using var owner = MemoryPool<byte>.Shared.Rent(buffer.Length);
var sliced = owner.Memory[..buffer.Length];
Task.Run(() => ReadChunkAsync(sliced, chunkIndex)).Wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use .GetAwaiter().GetResult() instead of .Wait() for better handling of exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Task.Run(() => ReadChunkAsync(sliced, chunkIndex)).Wait();
Task.Run(() => ReadChunkAsync(sliced, chunkIndex)).GetAwaiter().GetResult();

Comment on lines +3 to +13
public class IntegrationTestDataSource : DependencyInjectionDataSourceAttribute<IServiceProvider>
{
public override IServiceProvider CreateScope(DataGeneratorMetadata dataGeneratorMetadata)
{
throw new NotImplementedException();
}

public override object? Create(IServiceProvider scope, Type type)
{
throw new NotImplementedException();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What would this be for?

@github-actions github-actions bot removed the status-needs-rebase Set by CI do not remove label Oct 22, 2025
@github-actions
Copy link
Contributor

This PR doesn't conflict with main anymore. It can be merged after all status checks have passed and it has been reviewed.

@github-actions github-actions bot added the status-needs-rebase Set by CI do not remove label Nov 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2025

This PR conflicts with main. You need to rebase the PR before it can be merged.

@github-actions
Copy link
Contributor

This PR has been marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Stale status-needs-rebase Set by CI do not remove

Projects

Status: Development review

Development

Successfully merging this pull request may close these issues.

3 participants