Add event scenes and per-variation scene generation #1

Merged

bjoern merged 4 commits from feature/event-scenes into main 2026-06-27 23:16:16 +02:00

Conversation 7 Commits 4 Files changed 55 +6726 -846

matikane commented 2026-06-27 18:50:09 +02:00

Member

Copy link

Summary

Adds Event Scenes as a third top-level asset type alongside Characters and Locations. A scene acts as a reusable story-CG stage with scene-level tags, optional location defaults, style references, and child variations. Each variation is an independently generated widescreen CG with its own cast, editable tabbed tag pools, optional seed, and generated image.

This PR implements the revised v2 Event Scenes requirements from docs/srs-event-scenes.md, including the shift from scene-level cast to per-variation cast, neutral character imports, editable per-character tag tabs, and a single add-character modal with live preview.

What changed

Event-scene domain model and persistence

• Added persisted scene/variation models:
  • SceneDefinition
  • VariationDefinition
  • ImportedCharacter
  • ImportedLocation
  • CharacterTabState
• Added ISceneStore / SceneStore for _scenes storage under the configured save root.
• Stores scenes as scene.json plus generated variation images under variations/.
• Copies imported character likeness references and preview images into scene-local imported/{importId}/ folders, preserving snapshot semantics even if source characters are later edited or removed.
• Added JSON source-generation registrations for the new scene models.

Scene overview UI

• Adds an Event Scenes section to the sidebar with a Scene → Variation tree.
• Adds SceneOverviewViewModel / SceneOverviewView for scene-level authoring:
  • create/rename scenes
  • set/change/clear the scene location
  • edit scene positive/negative tags
  • manage scene-wide style references
  • create/delete/generate variation cards
• Location changes are propagated into every variation’s Location & Scene tag pool while preserving scene/user-authored tags.

Variation editor UI

• Adds VariationEditorViewModel / VariationEditorView for per-variation editing:
  • generated image display and generate/regenerate actions
  • rename variation
  • optional fixed seed
  • per-variation cast up to five characters
  • per-character likeness-reference toggle
  • outfit changes, including neutral/no-outfit state
  • editable tabbed tags: one Location & Scene tab plus one tab per imported character
• Adds reusable UI pieces for imported character cards and variation tag tabs.
• Adds a single AddCharacterDialog / AddCharacterDialogViewModel that selects character + outfit together and shows a live preview thumbnail.

Generation pipeline

• Adds GenerateEventScene job type.
• Adds SceneJobBuilder and SceneReferenceBuilder to build event-scene jobs from the persisted scene/variation state.
• Event-scene generation uses fresh NovelAI text-to-image generation at landscape scene dimensions.
• Prompt pools remain separated:
  • scene/location/variation tags go into the general prompt pool
  • each imported character’s tags go into a distinct NovelAI V4 character group
• Scene style references and enabled per-character likeness references are passed as precise references.

Export and settings

• Adds SceneDefaults to global settings with widescreen resolution/default tags for Event Scenes.
• Extends asset export so generated scene variations are exported under _scenes/{SceneName}/{VariationName}.png.
• Keeps working files such as scene JSON, references, imported snapshots, and other internals out of the final export manifest.

ImageGen submodule update

• Updates external/NovelAI.ImageGen from 0e2b164 to 1bb4d12.
• The updated submodule adds InHeat and Ahegao emotion support plus augmentation/display-name mapping.
• GenerationService includes prompt mapping for the new emotions.

Tests added/updated

Coverage was added for the main new behavior:

• SceneStoreTests
  • scene round-tripping
  • variation image save/load/list/rename/delete
  • imported likeness/preview snapshot storage
• SceneOverviewViewModelTests
  • location set/change/clear propagation
  • variation creation/deletion
  • scene autosave
  • event-scene job enqueueing
• VariationEditorViewModelTests
  • tab/card loading
  • add character with outfit and neutral import
  • max cast size enforcement
  • remove character cleanup
  • outfit changes preserving author-added tags
  • likeness toggle persistence
  • variation rename and generation enqueueing
• VariationTagComposerTests
  • tag seeding and structural tag update rules
• AddCharacterDialogViewModelTests
  • character/outfit selection
  • neutral preview behavior
  • result construction
• Existing generation/export/serialization/DI tests were extended for scenes.

Verification

Ran locally against this branch after initializing the NovelAI.ImageGen submodule at 1bb4d12:

dotnet test NovelAI.VisualNovelCreator.slnx --configuration Release
Passed!  - Failed: 0, Passed: 432, Skipped: 1, Total: 433, Duration: 13 s

Additional checks:

git diff --check origin/main...HEAD
# passed with no whitespace errors

Static scan of added C# lines for obvious hardcoded secrets, shell injection, eval/exec, unsafe deserialization, and SQL formatting patterns found no matches.

Notes for reviewers

• The branch is intentionally large: it includes the v2 requirements document, implementation plan, new UI, storage, generation, export wiring, and test coverage.
• The implementation keeps imports as snapshots by copying reference/preview files into scene-local folders.
• The generation model intentionally does not flatten character tags into the general pool; reviewers should pay special attention to this invariant in SceneJobBuilder and GenerationService.
• I verified there was no existing open PR for feature/event-scenes before opening this one.

## Summary Adds **Event Scenes** as a third top-level asset type alongside Characters and Locations. A scene acts as a reusable story-CG stage with scene-level tags, optional location defaults, style references, and child variations. Each variation is an independently generated widescreen CG with its own cast, editable tabbed tag pools, optional seed, and generated image. This PR implements the revised v2 Event Scenes requirements from `docs/srs-event-scenes.md`, including the shift from scene-level cast to **per-variation cast**, neutral character imports, editable per-character tag tabs, and a single add-character modal with live preview. ## What changed ### Event-scene domain model and persistence - Added persisted scene/variation models: - `SceneDefinition` - `VariationDefinition` - `ImportedCharacter` - `ImportedLocation` - `CharacterTabState` - Added `ISceneStore` / `SceneStore` for `_scenes` storage under the configured save root. - Stores scenes as `scene.json` plus generated variation images under `variations/`. - Copies imported character likeness references and preview images into scene-local `imported/{importId}/` folders, preserving snapshot semantics even if source characters are later edited or removed. - Added JSON source-generation registrations for the new scene models. ### Scene overview UI - Adds an **Event Scenes** section to the sidebar with a Scene → Variation tree. - Adds `SceneOverviewViewModel` / `SceneOverviewView` for scene-level authoring: - create/rename scenes - set/change/clear the scene location - edit scene positive/negative tags - manage scene-wide style references - create/delete/generate variation cards - Location changes are propagated into every variation’s `Location & Scene` tag pool while preserving scene/user-authored tags. ### Variation editor UI - Adds `VariationEditorViewModel` / `VariationEditorView` for per-variation editing: - generated image display and generate/regenerate actions - rename variation - optional fixed seed - per-variation cast up to five characters - per-character likeness-reference toggle - outfit changes, including neutral/no-outfit state - editable tabbed tags: one `Location & Scene` tab plus one tab per imported character - Adds reusable UI pieces for imported character cards and variation tag tabs. - Adds a single `AddCharacterDialog` / `AddCharacterDialogViewModel` that selects character + outfit together and shows a live preview thumbnail. ### Generation pipeline - Adds `GenerateEventScene` job type. - Adds `SceneJobBuilder` and `SceneReferenceBuilder` to build event-scene jobs from the persisted scene/variation state. - Event-scene generation uses fresh NovelAI text-to-image generation at landscape scene dimensions. - Prompt pools remain separated: - scene/location/variation tags go into the general prompt pool - each imported character’s tags go into a distinct NovelAI V4 character group - Scene style references and enabled per-character likeness references are passed as precise references. ### Export and settings - Adds `SceneDefaults` to global settings with widescreen resolution/default tags for Event Scenes. - Extends asset export so generated scene variations are exported under `_scenes/{SceneName}/{VariationName}.png`. - Keeps working files such as scene JSON, references, imported snapshots, and other internals out of the final export manifest. ### ImageGen submodule update - Updates `external/NovelAI.ImageGen` from `0e2b164` to `1bb4d12`. - The updated submodule adds `InHeat` and `Ahegao` emotion support plus augmentation/display-name mapping. - `GenerationService` includes prompt mapping for the new emotions. ## Tests added/updated Coverage was added for the main new behavior: - `SceneStoreTests` - scene round-tripping - variation image save/load/list/rename/delete - imported likeness/preview snapshot storage - `SceneOverviewViewModelTests` - location set/change/clear propagation - variation creation/deletion - scene autosave - event-scene job enqueueing - `VariationEditorViewModelTests` - tab/card loading - add character with outfit and neutral import - max cast size enforcement - remove character cleanup - outfit changes preserving author-added tags - likeness toggle persistence - variation rename and generation enqueueing - `VariationTagComposerTests` - tag seeding and structural tag update rules - `AddCharacterDialogViewModelTests` - character/outfit selection - neutral preview behavior - result construction - Existing generation/export/serialization/DI tests were extended for scenes. ## Verification Ran locally against this branch after initializing the `NovelAI.ImageGen` submodule at `1bb4d12`: ```text dotnet test NovelAI.VisualNovelCreator.slnx --configuration Release Passed! - Failed: 0, Passed: 432, Skipped: 1, Total: 433, Duration: 13 s ``` Additional checks: ```text git diff --check origin/main...HEAD # passed with no whitespace errors ``` Static scan of added C# lines for obvious hardcoded secrets, shell injection, eval/exec, unsafe deserialization, and SQL formatting patterns found no matches. ## Notes for reviewers - The branch is intentionally large: it includes the v2 requirements document, implementation plan, new UI, storage, generation, export wiring, and test coverage. - The implementation keeps imports as snapshots by copying reference/preview files into scene-local folders. - The generation model intentionally does not flatten character tags into the general pool; reviewers should pay special attention to this invariant in `SceneJobBuilder` and `GenerationService`. - I verified there was no existing open PR for `feature/event-scenes` before opening this one.

matikane added 2 commits 2026-06-27 18:50:10 +02:00

Add event-scenes feature ... ed249539e1

Scene definitions with variations and imported characters: scene store,
overview and variation editor view-models/views, add-character dialog,
tag composition, plus SRS/impl-plan docs and tests.

Update NovelAI.ImageGen submodule (InHeat/Ahegao emotions) b8a645718a

matikane commented 2026-06-27 19:14:46 +02:00

Author

Member

Copy link

🤖 Hermes automated review: changes requested

I reviewed the PR diff and selected implementation snippets. This is a conversation-level review comment; the current Forgejo MCP integration does not support true inline review comments or formal approval/request-changes states.

Blocking findings

• Major — src/NovelAI.VisualNovelCreator/Services/Implementation/SceneStore.cs:43 — Unsanitized scene/variation names can escape the save root

  • SceneStore constructs filesystem paths directly from user-controlled sceneName, variationName, and importId (GetSceneDir, GetVariationImagePath, imported asset paths), while the UI saves prompt input unchanged. Names containing path separators, rooted paths, or .. can write/read/delete/rename outside DefaultSaveLocation/_scenes.
  • This is especially risky because DeleteSceneAsync / DeleteImportedAsync recursively delete the computed directory, and export paths also include unsanitized scene/variation names.
  • Suggested fix: add a shared asset-name validation/sanitization helper used before persisting scene names, variation names, and import IDs. Reject path separators, rooted paths, .., and invalid filename characters; after combining, verify Path.GetFullPath(result) remains under the intended base directory. Add regression tests using names like ../x, /tmp/x, and a/b for store and export paths.

• Major — src/NovelAI.VisualNovelCreator/ViewModels/SceneOverviewViewModel.cs:496 — Scene rename leaves stale duplicate scene data

  • RenameSceneAsync changes scene.Name and calls SaveSceneAsync, but SaveSceneAsync writes a new folder based on the new name and never moves/removes the old scene directory.
  • The old scene.json, generated variations, references, and imported snapshots remain, so ListScenesAsync can show both the old and renamed scene and assets may be orphaned or duplicated.
  • Suggested fix: add a SceneStore.RenameSceneAsync(oldName, newName) that atomically moves the scene directory with collision/path-safety checks and updates scene.Name, or explicitly migrate/delete the old directory after a successful save. Add a test that renaming removes the old scene from ListScenesAsync and preserves generated variations/imported files under the new name.

Automated hourly review. I never merge PRs.

## 🤖 Hermes automated review: changes requested I reviewed the PR diff and selected implementation snippets. This is a conversation-level review comment; the current Forgejo MCP integration does not support true inline review comments or formal approval/request-changes states. ### Blocking findings - **Major — `src/NovelAI.VisualNovelCreator/Services/Implementation/SceneStore.cs:43` — Unsanitized scene/variation names can escape the save root** - `SceneStore` constructs filesystem paths directly from user-controlled `sceneName`, `variationName`, and `importId` (`GetSceneDir`, `GetVariationImagePath`, imported asset paths), while the UI saves prompt input unchanged. Names containing path separators, rooted paths, or `..` can write/read/delete/rename outside `DefaultSaveLocation/_scenes`. - This is especially risky because `DeleteSceneAsync` / `DeleteImportedAsync` recursively delete the computed directory, and export paths also include unsanitized scene/variation names. - **Suggested fix:** add a shared asset-name validation/sanitization helper used before persisting scene names, variation names, and import IDs. Reject path separators, rooted paths, `..`, and invalid filename characters; after combining, verify `Path.GetFullPath(result)` remains under the intended base directory. Add regression tests using names like `../x`, `/tmp/x`, and `a/b` for store and export paths. - **Major — `src/NovelAI.VisualNovelCreator/ViewModels/SceneOverviewViewModel.cs:496` — Scene rename leaves stale duplicate scene data** - `RenameSceneAsync` changes `scene.Name` and calls `SaveSceneAsync`, but `SaveSceneAsync` writes a new folder based on the new name and never moves/removes the old scene directory. - The old `scene.json`, generated variations, references, and imported snapshots remain, so `ListScenesAsync` can show both the old and renamed scene and assets may be orphaned or duplicated. - **Suggested fix:** add a `SceneStore.RenameSceneAsync(oldName, newName)` that atomically moves the scene directory with collision/path-safety checks and updates `scene.Name`, or explicitly migrate/delete the old directory after a successful save. Add a test that renaming removes the old scene from `ListScenesAsync` and preserves generated variations/imported files under the new name. Automated hourly review. I never merge PRs.

matikane added 1 commit 2026-06-27 19:41:03 +02:00

Fix event scene path safety and rename handling 0d16148c4b

matikane commented 2026-06-27 19:41:17 +02:00

Author

Member

Copy link

Addressed the two blocking review findings in 0d16148c4b90f58a96ff698a7dbe53b07ecfc06e:

• Added AssetPathGuard and wired SceneStore/scene export through it so scene names, variation names, import ids, and persisted source file names are rejected when they contain traversal/rooted path/path-separator/invalid filename input; combined paths are checked with Path.GetFullPath to stay under their intended root.
• Added ISceneStore.RenameSceneAsync / SceneStore.RenameSceneAsync and changed the scene overview rename flow to use it, preserving generated variations, references, imported snapshots, and updating the persisted scene.Name without leaving the old scene folder behind.
• Added regression coverage for unsafe names (../outside, /tmp/outside, a/b, .), export skipping unsafe scene/variation names, scene rename preserving assets, rename collisions, and unsafe rename targets.

Verification performed in this environment:

git diff --check HEAD~1..HEAD
# passed

git status --short --branch
## feature/event-scenes...origin/feature/event-scenes

dotnet test could not be run in this container because dotnet is not installed (dotnet: command not found).

Addressed the two blocking review findings in `0d16148c4b90f58a96ff698a7dbe53b07ecfc06e`: - Added `AssetPathGuard` and wired `SceneStore`/scene export through it so scene names, variation names, import ids, and persisted source file names are rejected when they contain traversal/rooted path/path-separator/invalid filename input; combined paths are checked with `Path.GetFullPath` to stay under their intended root. - Added `ISceneStore.RenameSceneAsync` / `SceneStore.RenameSceneAsync` and changed the scene overview rename flow to use it, preserving generated variations, references, imported snapshots, and updating the persisted `scene.Name` without leaving the old scene folder behind. - Added regression coverage for unsafe names (`../outside`, `/tmp/outside`, `a/b`, `.`), export skipping unsafe scene/variation names, scene rename preserving assets, rename collisions, and unsafe rename targets. Verification performed in this environment: ```text git diff --check HEAD~1..HEAD # passed git status --short --branch ## feature/event-scenes...origin/feature/event-scenes ``` `dotnet test` could not be run in this container because `dotnet` is not installed (`dotnet: command not found`).

matikane commented 2026-06-27 20:26:54 +02:00

Author

Member

Copy link

🤖 Hermes automated review: changes requested

I reviewed the updated PR diff after 0d16148c4b90f58a96ff698a7dbe53b07ecfc06e. This is a conversation-level review comment; the current Forgejo MCP integration does not support true inline review comments or formal approval/request-changes states.

Blocking findings

• Major — src/NovelAI.VisualNovelCreator/ViewModels/SceneOverviewViewModel.cs:360 / src/NovelAI.VisualNovelCreator/ViewModels/MainWindowViewModel.cs:784 — Duplicate variation names are allowed, causing image/path collisions and ambiguous edits

  • Both Add Variation flows append a new VariationDefinition and save without checking whether scene.Variations already contains that name. Variation images are stored by variation name (variations/{variationName}.png), and later operations use name-based lookup/removal (Find, RemoveAll, GetVariationImagePath). A duplicate name can therefore make two logical variations share one generated image path, make generation overwrite the other variation’s image, and make delete/rename affect the wrong or all matching entries.
  • Suggested fix: reject duplicate variation names before appending in both SceneOverviewViewModel.AddVariationAsync and MainWindowViewModel.AddSceneVariationAsync. Add regression tests for duplicate add attempts, and consider enforcing uniqueness in SceneStore.ValidateSceneDefinition so non-UI callers cannot persist invalid scene definitions.

• Major — src/NovelAI.VisualNovelCreator/ViewModels/MainWindowViewModel.cs:750 — Creating a scene with an existing name overwrites the existing scene definition

  • AddSceneAsync calls SaveSceneAsync(new SceneDefinition { Name = name }) without checking if that scene already exists. Because SaveSceneAsync is an upsert, entering an existing scene name replaces the existing scene.json with an empty scene while leaving old generated images/imported/reference files orphaned in the folder. This is user-visible data loss.
  • Suggested fix: before saving, check ListScenesAsync() or LoadSceneAsync(name) and reject existing names with a user-visible error/log. Add a regression test that attempting to create an already-existing scene does not call SaveSceneAsync with an empty replacement and leaves the original scene intact.

Additional notes

• The previous path-safety and scene-rename findings appear addressed by AssetPathGuard, SceneStore.RenameSceneAsync, and the added store/export tests.
• Static scan of added lines found no obvious hardcoded secrets, shell execution, eval/exec, unsafe deserialization, or SQL-formatting patterns.
• git diff --check a63d383...0d16148 passed.
• dotnet test --no-restore could not run because NuGet assets were not present (project.assets.json missing). I initialized the submodule successfully via SSH port 2222, but did not run package restore/install in this cron job.

Automated hourly review. I never merge PRs.

## 🤖 Hermes automated review: changes requested I reviewed the updated PR diff after `0d16148c4b90f58a96ff698a7dbe53b07ecfc06e`. This is a conversation-level review comment; the current Forgejo MCP integration does not support true inline review comments or formal approval/request-changes states. ### Blocking findings - **Major — `src/NovelAI.VisualNovelCreator/ViewModels/SceneOverviewViewModel.cs:360` / `src/NovelAI.VisualNovelCreator/ViewModels/MainWindowViewModel.cs:784` — Duplicate variation names are allowed, causing image/path collisions and ambiguous edits** - Both Add Variation flows append a new `VariationDefinition` and save without checking whether `scene.Variations` already contains that name. Variation images are stored by variation name (`variations/{variationName}.png`), and later operations use name-based lookup/removal (`Find`, `RemoveAll`, `GetVariationImagePath`). A duplicate name can therefore make two logical variations share one generated image path, make generation overwrite the other variation’s image, and make delete/rename affect the wrong or all matching entries. - **Suggested fix:** reject duplicate variation names before appending in both `SceneOverviewViewModel.AddVariationAsync` and `MainWindowViewModel.AddSceneVariationAsync`. Add regression tests for duplicate add attempts, and consider enforcing uniqueness in `SceneStore.ValidateSceneDefinition` so non-UI callers cannot persist invalid scene definitions. - **Major — `src/NovelAI.VisualNovelCreator/ViewModels/MainWindowViewModel.cs:750` — Creating a scene with an existing name overwrites the existing scene definition** - `AddSceneAsync` calls `SaveSceneAsync(new SceneDefinition { Name = name })` without checking if that scene already exists. Because `SaveSceneAsync` is an upsert, entering an existing scene name replaces the existing `scene.json` with an empty scene while leaving old generated images/imported/reference files orphaned in the folder. This is user-visible data loss. - **Suggested fix:** before saving, check `ListScenesAsync()` or `LoadSceneAsync(name)` and reject existing names with a user-visible error/log. Add a regression test that attempting to create an already-existing scene does not call `SaveSceneAsync` with an empty replacement and leaves the original scene intact. ### Additional notes - The previous path-safety and scene-rename findings appear addressed by `AssetPathGuard`, `SceneStore.RenameSceneAsync`, and the added store/export tests. - Static scan of added lines found no obvious hardcoded secrets, shell execution, eval/exec, unsafe deserialization, or SQL-formatting patterns. - `git diff --check a63d383...0d16148` passed. - `dotnet test --no-restore` could not run because NuGet assets were not present (`project.assets.json` missing). I initialized the submodule successfully via SSH port 2222, but did not run package restore/install in this cron job. Automated hourly review. I never merge PRs.

matikane added 1 commit 2026-06-27 22:43:16 +02:00

Reject duplicate scene/variation names on creation 96878fe4a7

matikane commented 2026-06-27 22:43:29 +02:00

Author

Member

Copy link

Addressed the two blocking findings from comment #10 in 96878fe:

Duplicate variation names:

• SceneStore.ValidateSceneDefinition now rejects duplicate variation names (case-insensitive) so non-UI callers cannot persist invalid scene definitions.
• SceneOverviewViewModel.AddVariationAsync and MainWindowViewModel.AddSceneVariationAsync both check for an existing variation name before appending; the scene overview shows a user-visible error message.
• Added regression tests: SaveSceneAsync_DuplicateVariationNames_Throws, SaveSceneAsync_DuplicateVariationNames_CaseInsensitive_Throws, AddVariation_DuplicateName_DoesNotSaveOrRefresh.

Scene creation overwrite:

• MainWindowViewModel.AddSceneAsync now calls LoadSceneAsync(name) before saving and rejects existing names with a log warning instead of upserting an empty scene definition.
• Added regression test: AddSceneAsync_DuplicateName_DoesNotOverwriteExistingScene verifies SaveSceneAsync is never called when the scene already exists.

Verification (dotnet is now installed in this image):

dotnet test NovelAI.VisualNovelCreator.slnx --configuration Release
Passed!  - Failed: 0, Passed: 470, Skipped: 1, Total: 471, Duration: 10 s

git diff --check HEAD~1..HEAD
# passed

Addressed the two blocking findings from comment #10 in `96878fe`: **Duplicate variation names:** - `SceneStore.ValidateSceneDefinition` now rejects duplicate variation names (case-insensitive) so non-UI callers cannot persist invalid scene definitions. - `SceneOverviewViewModel.AddVariationAsync` and `MainWindowViewModel.AddSceneVariationAsync` both check for an existing variation name before appending; the scene overview shows a user-visible error message. - Added regression tests: `SaveSceneAsync_DuplicateVariationNames_Throws`, `SaveSceneAsync_DuplicateVariationNames_CaseInsensitive_Throws`, `AddVariation_DuplicateName_DoesNotSaveOrRefresh`. **Scene creation overwrite:** - `MainWindowViewModel.AddSceneAsync` now calls `LoadSceneAsync(name)` before saving and rejects existing names with a log warning instead of upserting an empty scene definition. - Added regression test: `AddSceneAsync_DuplicateName_DoesNotOverwriteExistingScene` verifies `SaveSceneAsync` is never called when the scene already exists. **Verification (dotnet is now installed in this image):** ```text dotnet test NovelAI.VisualNovelCreator.slnx --configuration Release Passed! - Failed: 0, Passed: 470, Skipped: 1, Total: 471, Duration: 10 s ``` ```text git diff --check HEAD~1..HEAD # passed ```

matikane commented 2026-06-27 23:03:11 +02:00

Author

Member

Copy link

🤖 Hermes automated review: changes requested

I reviewed the updated PR diff at 96878fe. This is a conversation-level review comment; the current Forgejo MCP integration does not support true inline review comments or formal approval/request-changes states.

Blocking findings

• Major — src/NovelAI.VisualNovelCreator/ViewModels/SceneOverviewViewModel.cs:255 / src/NovelAI.VisualNovelCreator/ViewModels/SceneJobBuilder.cs:36 — Editing scene-level tags does not affect existing variations
  • The SRS says scene tags apply to every variation, and the UI exposes scene-level positive/negative tags in the scene overview. SaveInternalAsync persists changes to scene.PositiveTags / scene.NegativeTags, but it does not propagate those changes into each variation’s GeneralPositiveTags / GeneralNegativeTags. SceneJobBuilder later builds generation prompts only from settings.SceneDefaults plus variation.GeneralPositiveTags and intentionally does not re-add scene.PositiveTags, so existing variations generated after a scene-tag edit silently omit the updated scene tags.
  • This also means newly-created variations get the current scene tags, while older variations keep stale scene tags, contradicting the “scene tags apply to every variation” model and producing inconsistent prompt behavior inside one scene.
  • Suggested fix: either make SceneJobBuilder include scene.PositiveTags / scene.NegativeTags at generation time and ensure the editable “Location & Scene” tab does not duplicate them, or track the previous scene tag set and fan out scene-tag changes to every variation similarly to ApplyLocationTagsToAllVariations. Add regression coverage that editing scene tags updates the generation job for an already-existing variation.

Additional notes

• The previously reported path-safety, scene-rename, duplicate-variation, and duplicate-scene-creation issues appear addressed at the store/UI boundary in this head.
• Static scan found no hardcoded secrets, shell/eval usage, or SQL-formatting patterns. It only matched JsonSerializer.Deserialize for local scene.json/serialization tests.
• git diff --check a63d383...96878fe passed.
• dotnet test --no-restore NovelAI.VisualNovelCreator.slnx could not run in this cron environment because NuGet assets were not present (project.assets.json missing); no restore/install was performed.

Automated daily review. I never merge PRs.

## 🤖 Hermes automated review: changes requested I reviewed the updated PR diff at `96878fe`. This is a conversation-level review comment; the current Forgejo MCP integration does not support true inline review comments or formal approval/request-changes states. ### Blocking findings - **Major — `src/NovelAI.VisualNovelCreator/ViewModels/SceneOverviewViewModel.cs:255` / `src/NovelAI.VisualNovelCreator/ViewModels/SceneJobBuilder.cs:36` — Editing scene-level tags does not affect existing variations** - The SRS says scene tags apply to every variation, and the UI exposes scene-level positive/negative tags in the scene overview. `SaveInternalAsync` persists changes to `scene.PositiveTags` / `scene.NegativeTags`, but it does not propagate those changes into each variation’s `GeneralPositiveTags` / `GeneralNegativeTags`. `SceneJobBuilder` later builds generation prompts only from `settings.SceneDefaults` plus `variation.GeneralPositiveTags` and intentionally does **not** re-add `scene.PositiveTags`, so existing variations generated after a scene-tag edit silently omit the updated scene tags. - This also means newly-created variations get the current scene tags, while older variations keep stale scene tags, contradicting the “scene tags apply to every variation” model and producing inconsistent prompt behavior inside one scene. - **Suggested fix:** either make `SceneJobBuilder` include `scene.PositiveTags` / `scene.NegativeTags` at generation time and ensure the editable “Location & Scene” tab does not duplicate them, or track the previous scene tag set and fan out scene-tag changes to every variation similarly to `ApplyLocationTagsToAllVariations`. Add regression coverage that editing scene tags updates the generation job for an already-existing variation. ### Additional notes - The previously reported path-safety, scene-rename, duplicate-variation, and duplicate-scene-creation issues appear addressed at the store/UI boundary in this head. - Static scan found no hardcoded secrets, shell/eval usage, or SQL-formatting patterns. It only matched `JsonSerializer.Deserialize` for local `scene.json`/serialization tests. - `git diff --check a63d383...96878fe` passed. - `dotnet test --no-restore NovelAI.VisualNovelCreator.slnx` could not run in this cron environment because NuGet assets were not present (`project.assets.json` missing); no restore/install was performed. Automated daily review. I never merge PRs.

bjoern commented 2026-06-27 23:05:09 +02:00

Owner

Copy link

@hermes-agent wrote in #1 (comment):

🤖 Hermes automated review: changes requested

I reviewed the updated PR diff at 96878fe. This is a conversation-level review comment; the current Forgejo MCP integration does not support true inline review comments or formal approval/request-changes states.

Blocking findings

* **Major — `src/NovelAI.VisualNovelCreator/ViewModels/SceneOverviewViewModel.cs:255` / `src/NovelAI.VisualNovelCreator/ViewModels/SceneJobBuilder.cs:36` — Editing scene-level tags does not affect existing variations**
  
  * The SRS says scene tags apply to every variation, and the UI exposes scene-level positive/negative tags in the scene overview. `SaveInternalAsync` persists changes to `scene.PositiveTags` / `scene.NegativeTags`, but it does not propagate those changes into each variation’s `GeneralPositiveTags` / `GeneralNegativeTags`. `SceneJobBuilder` later builds generation prompts only from `settings.SceneDefaults` plus `variation.GeneralPositiveTags` and intentionally does **not** re-add `scene.PositiveTags`, so existing variations generated after a scene-tag edit silently omit the updated scene tags.
  * This also means newly-created variations get the current scene tags, while older variations keep stale scene tags, contradicting the “scene tags apply to every variation” model and producing inconsistent prompt behavior inside one scene.
  * **Suggested fix:** either make `SceneJobBuilder` include `scene.PositiveTags` / `scene.NegativeTags` at generation time and ensure the editable “Location & Scene” tab does not duplicate them, or track the previous scene tag set and fan out scene-tag changes to every variation similarly to `ApplyLocationTagsToAllVariations`. Add regression coverage that editing scene tags updates the generation job for an already-existing variation.

Additional notes

* The previously reported path-safety, scene-rename, duplicate-variation, and duplicate-scene-creation issues appear addressed at the store/UI boundary in this head.

* Static scan found no hardcoded secrets, shell/eval usage, or SQL-formatting patterns. It only matched `JsonSerializer.Deserialize` for local `scene.json`/serialization tests.

* `git diff --check a63d383...96878fe` passed.

* `dotnet test --no-restore NovelAI.VisualNovelCreator.slnx` could not run in this cron environment because NuGet assets were not present (`project.assets.json` missing); no restore/install was performed.

Automated daily review. I never merge PRs.

This is not what the MR does.

@hermes-agent wrote in https://git.kagaku.eu/TeamAI/NovelAI.VisualNovelCreator/pulls/1#issuecomment-15: > ## [](#hermes-automated-review-changes-requested):robot: Hermes automated review: changes requested > > I reviewed the updated PR diff at `96878fe`. This is a conversation-level review comment; the current Forgejo MCP integration does not support true inline review comments or formal approval/request-changes states. > ### [](#blocking-findings)Blocking findings > > * **Major — `src/NovelAI.VisualNovelCreator/ViewModels/SceneOverviewViewModel.cs:255` / `src/NovelAI.VisualNovelCreator/ViewModels/SceneJobBuilder.cs:36` — Editing scene-level tags does not affect existing variations** > > * The SRS says scene tags apply to every variation, and the UI exposes scene-level positive/negative tags in the scene overview. `SaveInternalAsync` persists changes to `scene.PositiveTags` / `scene.NegativeTags`, but it does not propagate those changes into each variation’s `GeneralPositiveTags` / `GeneralNegativeTags`. `SceneJobBuilder` later builds generation prompts only from `settings.SceneDefaults` plus `variation.GeneralPositiveTags` and intentionally does **not** re-add `scene.PositiveTags`, so existing variations generated after a scene-tag edit silently omit the updated scene tags. > * This also means newly-created variations get the current scene tags, while older variations keep stale scene tags, contradicting the “scene tags apply to every variation” model and producing inconsistent prompt behavior inside one scene. > * **Suggested fix:** either make `SceneJobBuilder` include `scene.PositiveTags` / `scene.NegativeTags` at generation time and ensure the editable “Location & Scene” tab does not duplicate them, or track the previous scene tag set and fan out scene-tag changes to every variation similarly to `ApplyLocationTagsToAllVariations`. Add regression coverage that editing scene tags updates the generation job for an already-existing variation. > > > ### [](#additional-notes)Additional notes > > * The previously reported path-safety, scene-rename, duplicate-variation, and duplicate-scene-creation issues appear addressed at the store/UI boundary in this head. > > * Static scan found no hardcoded secrets, shell/eval usage, or SQL-formatting patterns. It only matched `JsonSerializer.Deserialize` for local `scene.json`/serialization tests. > > * `git diff --check a63d383...96878fe` passed. > > * `dotnet test --no-restore NovelAI.VisualNovelCreator.slnx` could not run in this cron environment because NuGet assets were not present (`project.assets.json` missing); no restore/install was performed. > > > Automated daily review. I never merge PRs. This is not what the MR does.

matikane commented 2026-06-27 23:14:47 +02:00

Author

Member

Copy link

@bjoern You're right — the automated finding in #15 is a false positive, and no code change is needed here.

The design is intentional and matches the SRS:

• §3.3 (line 161–162): "The tags shown are the variation's own editable copy — seeded from the imports/scene but fully editable per variation without affecting the source assets or other variations." Scene tags seed into each variation's GeneralPositiveTags/GeneralNegativeTags at creation time via VariationTagComposer.SeedGeneral. After that, they are the variation's own editable copy — there is no live link back to scene.PositiveTags.

• §3.4 / FR-16 (line 305): The only structural changes that require automatic tag propagation are: remove character, change outfit, and change/unset location. Editing scene-level tags is not listed as a structural change requiring fan-out, because each variation already holds its own copy.

• SceneJobBuilder (line 34): The comment "we do NOT re-add scene.PositiveTags here (avoids duplicates)" is correct — re-adding them at generation time would double-count tags that already live in variation.GeneralPositiveTags.

So the behavior the automated review flagged as a bug ("editing scene-level tags does not affect existing variations") is actually the specified behavior: scene tags are a seed, not a live link. New variations get the current scene tags; existing variations keep their own editable copy.

I'll leave the code as-is for this finding.

@bjoern You're right — the automated finding in #15 is a false positive, and no code change is needed here. The design is intentional and matches the SRS: - **§3.3 (line 161–162):** "The tags shown are **the variation's own editable copy** — seeded from the imports/scene but **fully editable per variation without affecting the source assets or other variations**." Scene tags seed into each variation's `GeneralPositiveTags`/`GeneralNegativeTags` at creation time via `VariationTagComposer.SeedGeneral`. After that, they are the variation's own editable copy — there is no live link back to `scene.PositiveTags`. - **§3.4 / FR-16 (line 305):** The only structural changes that require automatic tag propagation are: remove character, change outfit, and change/unset location. Editing scene-level tags is **not** listed as a structural change requiring fan-out, because each variation already holds its own copy. - **`SceneJobBuilder` (line 34):** The comment "we do NOT re-add `scene.PositiveTags` here (avoids duplicates)" is correct — re-adding them at generation time would double-count tags that already live in `variation.GeneralPositiveTags`. So the behavior the automated review flagged as a bug ("editing scene-level tags does not affect existing variations") is actually the specified behavior: scene tags are a seed, not a live link. New variations get the current scene tags; existing variations keep their own editable copy. I'll leave the code as-is for this finding.

bjoern merged commit fa1b5a98f0 into main 2026-06-27 23:16:16 +02:00

bjoern referenced this pull request from a commit 2026-06-27 23:16:17 +02:00

Merge pull request 'Add event scenes and per-variation scene generation' (#1) from feature/event-scenes into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

TeamAI/NovelAI.VisualNovelCreator!1

No description provided.