Config Updates - User settings - Saving Methods
This commit is contained in:
107
ProjectNotes/ConfigurationRefactoringPlan.md
Normal file
107
ProjectNotes/ConfigurationRefactoringPlan.md
Normal file
@@ -0,0 +1,107 @@
|
||||
# Configuration System Refactoring Plan
|
||||
|
||||
This document outlines the plan for refactoring the configuration system of the Asset Processor Tool.
|
||||
|
||||
## Overall Goals
|
||||
|
||||
1. **Decouple Definitions:** Separate `ASSET_TYPE_DEFINITIONS` and `FILE_TYPE_DEFINITIONS` from the main `config/app_settings.json` into dedicated files.
|
||||
2. **Introduce User Overrides:** Allow users to override base settings via a new `config/user_settings.json` file.
|
||||
3. **Improve GUI Saving:** (Lower Priority) Make GUI configuration saving more targeted to avoid overwriting unrelated settings when saving changes from `ConfigEditorDialog` or `LLMEditorWidget`.
|
||||
|
||||
## Proposed Plan Phases
|
||||
|
||||
**Phase 1: Decouple Definitions**
|
||||
|
||||
1. **Create New Definition Files:**
|
||||
* Create `config/asset_type_definitions.json`.
|
||||
* Create `config/file_type_definitions.json`.
|
||||
2. **Migrate Content:**
|
||||
* Move `ASSET_TYPE_DEFINITIONS` object from `config/app_settings.json` to `config/asset_type_definitions.json`.
|
||||
* Move `FILE_TYPE_DEFINITIONS` object from `config/app_settings.json` to `config/file_type_definitions.json`.
|
||||
3. **Update `configuration.py`:**
|
||||
* Add constants for new definition file paths.
|
||||
* Modify `Configuration` class to load these new files.
|
||||
* Update property methods (e.g., `get_asset_type_definitions`, `get_file_type_definitions_with_examples`) to use data from the new definition dictionaries.
|
||||
* Adjust validation (`_validate_configs`) as needed.
|
||||
4. **Update GUI & `load_base_config()`:**
|
||||
* Modify `load_base_config()` to load and return a combined dictionary including `app_settings.json` and the two new definition files.
|
||||
* Update GUI components relying on `load_base_config()` to ensure they receive the necessary definition data.
|
||||
|
||||
**Phase 2: Implement User Overrides**
|
||||
|
||||
1. **Define `user_settings.json`:**
|
||||
* Establish `config/user_settings.json` for user-specific overrides, mirroring parts of `app_settings.json`.
|
||||
2. **Update `configuration.py` Loading:**
|
||||
* In `Configuration.__init__`, load `app_settings.json`, then definition files, then attempt to load and deep merge `user_settings.json` (user settings override base).
|
||||
* Load presets *after* the base+user merge (presets override combined base+user).
|
||||
* Modify `load_base_config()` to also load and merge `user_settings.json` after `app_settings.json`.
|
||||
3. **Update GUI Editors:**
|
||||
* Modify `ConfigEditorDialog` to load the effective settings (base+user) but save changes *only* to `config/user_settings.json`.
|
||||
* `LLMEditorWidget` continues targeting `llm_settings.json`.
|
||||
|
||||
**Phase 3: Granular GUI Saving (Lower Priority)**
|
||||
|
||||
1. **Refactor Saving Logic:**
|
||||
* In `ConfigEditorDialog` and `LLMEditorWidget`:
|
||||
* Load the current target file (`user_settings.json` or `llm_settings.json`).
|
||||
* Identify specific setting(s) changed by the user in the GUI session.
|
||||
* Update only those specific key(s) in the loaded dictionary.
|
||||
* Write the entire modified dictionary back to the target file, preserving untouched settings.
|
||||
|
||||
## Proposed File Structure & Loading Flow
|
||||
|
||||
```mermaid
|
||||
graph LR
|
||||
subgraph Config Files
|
||||
A[config/asset_type_definitions.json]
|
||||
B[config/file_type_definitions.json]
|
||||
C[config/app_settings.json (Base Defaults)]
|
||||
D[config/user_settings.json (User Overrides)]
|
||||
E[config/llm_settings.json]
|
||||
F[config/suppliers.json]
|
||||
G[Presets/*.json]
|
||||
end
|
||||
|
||||
subgraph Code
|
||||
H[configuration.py]
|
||||
I[GUI]
|
||||
J[Processing Engine / Pipeline]
|
||||
K[LLM Handlers]
|
||||
end
|
||||
|
||||
subgraph Loading Flow (Configuration Class)
|
||||
L(Load Asset Types) --> H
|
||||
M(Load File Types) --> H
|
||||
N(Load Base Settings) --> P(Merge Base + User)
|
||||
O(Load User Settings) --> P
|
||||
P --> R(Merge Preset Overrides)
|
||||
Q(Load LLM Settings) --> H
|
||||
R --> T(Final Config Object)
|
||||
G -- Load Preset --> R
|
||||
H -- Contains --> T
|
||||
end
|
||||
|
||||
subgraph Loading Flow (GUI - load_base_config)
|
||||
L2(Load Asset Types) --> U(Return Merged Defaults + Defs)
|
||||
M2(Load File Types) --> U
|
||||
N2(Load Base Settings) --> V(Merge Base + User)
|
||||
O2(Load User Settings) --> V
|
||||
V --> U
|
||||
I -- Calls --> U
|
||||
end
|
||||
|
||||
|
||||
T -- Used by --> J
|
||||
T -- Used by --> K
|
||||
|
||||
I -- Edits --> D
|
||||
I -- Edits --> E
|
||||
I -- Manages --> F
|
||||
|
||||
style A fill:#f9f,stroke:#333,stroke-width:2px
|
||||
style B fill:#f9f,stroke:#333,stroke-width:2px
|
||||
style C fill:#ccf,stroke:#333,stroke-width:2px
|
||||
style D fill:#9cf,stroke:#333,stroke-width:2px
|
||||
style E fill:#ccf,stroke:#333,stroke-width:2px
|
||||
style F fill:#9cf,stroke:#333,stroke-width:2px
|
||||
style G fill:#ffc,stroke:#333,stroke-width:2px
|
||||
@@ -1,96 +0,0 @@
|
||||
# Plan: Enforcing "MAP_" Prefix for Internal Processing and Standard Type for Output Naming
|
||||
|
||||
**Date:** 2025-05-13
|
||||
|
||||
**I. Goal:**
|
||||
The primary goal is to ensure that for all internal processing, the system *exclusively* uses `FileRule.item_type` values that start with the "MAP_" prefix (e.g., "MAP_COL", "MAP_NRM"). The "standard type" (e.g., "COL", "NRM") associated with these "MAP_" types (as defined in `config/app_settings.json`) should *only* be used during the file saving stages for output naming. Any `FileRule` whose `item_type` does not start with "MAP_" (and isn't a special type like "EXTRA" or "MODEL") should be skipped by the relevant map processing stages.
|
||||
|
||||
**II. Current State Analysis Summary:**
|
||||
|
||||
* **Output Naming:** The use of "standard type" for output filenames via the `get_filename_friendly_map_type` utility in `SaveVariantsStage` and `OutputOrganizationStage` is **correct** and already meets the requirement.
|
||||
* **Internal "MAP_" Prefix Usage:**
|
||||
* Some stages like `GlossToRoughConversionStage` correctly check for "MAP_" prefixes (e.g., `processing_map_type.startswith("MAP_GLOSS")`).
|
||||
* Other stages like `RegularMapProcessorStage` and `MergedTaskProcessorStage` (and its helpers) implicitly expect "MAP_" prefixed types for their internal regex-based logic but lack explicit checks to skip items if the prefix is missing.
|
||||
* Stages like `AlphaExtractionToMaskStage` and `NormalMapGreenChannelStage` currently use non-"MAP_" prefixed "standard types" (e.g., "NORMAL", "ALBEDO") when reading from `context.processed_maps_details` for their decision-making logic.
|
||||
* The `PrepareProcessingItemsStage` adds `FileRule`s to the processing queue without filtering based on the "MAP_" prefix in `item_type`.
|
||||
* **Data Consistency in `AssetProcessingContext`:**
|
||||
* `FileRule.item_type` is the field that should hold the "MAP_" prefixed type from the initial rule generation.
|
||||
* `context.processed_maps_details` entries can contain various map type representations:
|
||||
* `map_type`: Often stores the "standard type" (e.g., "Roughness", "MASK", "NORMAL").
|
||||
* `processing_map_type` / `internal_map_type`: Generally seem to store the "MAP_" prefixed type. This needs to be consistent.
|
||||
* **Configuration (`config/app_settings.json`):**
|
||||
* `FILE_TYPE_DEFINITIONS` correctly use "MAP_" prefixed keys.
|
||||
* `MAP_MERGE_RULES` need to be reviewed to ensure their `output_map_type` and input map types are "MAP_" prefixed.
|
||||
|
||||
**III. Proposed Changes (Code Identification & Recommendations):**
|
||||
|
||||
**A. Enforce "MAP_" Prefix for Processing Items (Skipping Logic):**
|
||||
The core requirement is that processing stages should skip `FileRule` items if their `item_type` doesn't start with "MAP_".
|
||||
|
||||
1. **`RegularMapProcessorStage` (`processing/pipeline/stages/regular_map_processor.py`):**
|
||||
* **Identify:** In the `execute` method, `initial_internal_map_type` is derived from `file_rule.item_type_override` or `file_rule.item_type`.
|
||||
* **Recommend:** Add an explicit check after determining `initial_internal_map_type`. If `initial_internal_map_type` does not start with `"MAP_"`, the stage should log a warning, set the `result.status` to "Skipped (Invalid Type)" or similar, and return `result` early, effectively skipping processing for this item.
|
||||
|
||||
2. **`MergedTaskProcessorStage` (`processing/pipeline/stages/merged_task_processor.py`):**
|
||||
* **Identify:** This stage processes `MergeTaskDefinition`s. The definitions for these tasks (input types, output type) come from `MAP_MERGE_RULES` in `config/app_settings.json`. The stage uses `required_map_type_from_rule` for its inputs.
|
||||
* **Recommend:**
|
||||
* **Configuration First:** Review all entries in `MAP_MERGE_RULES` in `config/app_settings.json`.
|
||||
* Ensure the `output_map_type` for each rule (e.g., "MAP_NRMRGH") starts with "MAP_".
|
||||
* Ensure all map type values within the `inputs` dictionary (e.g., `"R": "MAP_NRM"`) start with "MAP_".
|
||||
* **Stage Logic:** In the `execute` method, when iterating through `merge_inputs_config.items()`, check if `required_map_type_from_rule` starts with `"MAP_"`. If not, log a warning and either:
|
||||
* Skip loading/processing this specific input channel (potentially using its fallback if the overall merge can still proceed).
|
||||
* Or, if a non-"MAP_" input is critical, fail the entire merge task for this asset.
|
||||
* The helper `_apply_in_memory_transformations` already uses regex expecting "MAP_" prefixes; this will naturally fail or misbehave if inputs are not "MAP_" prefixed, reinforcing the need for the check above.
|
||||
|
||||
**B. Standardize Map Type Fields and Usage in `context.processed_maps_details`:**
|
||||
Ensure consistency in how "MAP_" prefixed types are stored and accessed within `context.processed_maps_details` for internal logic (not naming).
|
||||
|
||||
1. **Recommendation:** Establish a single, consistent field name within `context.processed_maps_details` to store the definitive "MAP_" prefixed internal map type (e.g., `internal_map_type` or `processing_map_type`). All stages that perform logic based on the specific *kind* of map (e.g., transformations, source selection) should read from this standardized field. The `map_type` field can continue to store the "standard type" (e.g., "Roughness") primarily for informational/metadata purposes if needed, but not for core processing logic.
|
||||
|
||||
2. **`AlphaExtractionToMaskStage` (`processing/pipeline/stages/alpha_extraction_to_mask.py`):**
|
||||
* **Identify:**
|
||||
* Checks for existing MASK map using `file_rule.map_type == "MASK"`. (Discrepancy: `FileRule` uses `item_type`).
|
||||
* Searches for suitable source maps using `details.get('map_type') in self.SUITABLE_SOURCE_MAP_TYPES` where `SUITABLE_SOURCE_MAP_TYPES` are standard types like "ALBEDO".
|
||||
* When adding new details, it sets `map_type: "MASK"` and the new `FileRule` gets `item_type="MAP_MASK"`.
|
||||
* **Recommend:**
|
||||
* Change the check for an existing MASK map to `file_rule.item_type == "MAP_MASK"`.
|
||||
* Modify the source map search to use the standardized "MAP_" prefixed field from `details` (e.g., `details.get('internal_map_type')`) and update `SUITABLE_SOURCE_MAP_TYPES` to be "MAP_" prefixed (e.g., "MAP_COL", "MAP_ALBEDO").
|
||||
* When adding new details for the created MASK map to `context.processed_maps_details`, ensure the standardized "MAP_" prefixed field is set to "MAP_MASK", and `map_type` (if kept) is "MASK".
|
||||
|
||||
3. **`NormalMapGreenChannelStage` (`processing/pipeline/stages/normal_map_green_channel.py`):**
|
||||
* **Identify:** Checks `map_details.get('map_type') == "NORMAL"`.
|
||||
* **Recommend:** Change this check to use the standardized "MAP_" prefixed field from `map_details` (e.g., `map_details.get('internal_map_type')`) and verify if it `startswith("MAP_NRM")`.
|
||||
|
||||
4. **`GlossToRoughConversionStage` (`processing/pipeline/stages/gloss_to_rough_conversion.py`):**
|
||||
* **Identify:** This stage already uses `processing_map_type.startswith("MAP_GLOSS")` and updates `processing_map_type` to "MAP_ROUGH" in `map_details`. It also updates the `FileRule.item_type` correctly.
|
||||
* **Recommend:** This stage is largely consistent. Ensure the field it reads/writes (`processing_map_type`) aligns with the chosen standardized "MAP_" prefixed field for `processed_maps_details`.
|
||||
|
||||
**C. Review Orchestration Logic (Conceptual):**
|
||||
* When the orchestrator populates `context.processed_maps_details` after stages like `SaveVariantsStage`, ensure it stores the "MAP_" prefixed `internal_map_type` (from `SaveVariantsInput`) into the chosen standardized field in `processed_maps_details`.
|
||||
|
||||
**IV. Testing Recommendations:**
|
||||
|
||||
* Create test cases with `AssetRule`s containing `FileRule`s where `item_type` is intentionally set to a non-"MAP_" prefixed value (e.g., "COLOR_MAP", "TEXTURE_ROUGH"). Verify that `RegularMapProcessorStage` skips these.
|
||||
* Modify `MAP_MERGE_RULES` in a test configuration:
|
||||
* Set an `output_map_type` to a non-"MAP_" value.
|
||||
* Set an input map type (e.g., for channel "R") to a non-"MAP_" value.
|
||||
* Verify that `MergedTaskProcessorStage` correctly handles these (e.g., fails the task, skips the input, logs warnings).
|
||||
* Test `AlphaExtractionToMaskStage`:
|
||||
* With an existing `FileRule` having `item_type="MAP_MASK"` to ensure extraction is skipped.
|
||||
* With source maps having "MAP_COL" (with alpha) as their `internal_map_type` in `processed_maps_details` to ensure they are correctly identified as sources.
|
||||
* Test `NormalMapGreenChannelStage` with a normal map having "MAP_NRM" as its `internal_map_type` in `processed_maps_details` to ensure it's processed.
|
||||
* Verify that output filenames continue to use the "standard type" (e.g., "COL", "ROUGH", "NRM") correctly.
|
||||
|
||||
**V. Mermaid Diagram (Illustrative Flow for `FileRule` Processing):**
|
||||
|
||||
```mermaid
|
||||
graph TD
|
||||
A[AssetRule with FileRules] --> B{FileRuleFilterStage};
|
||||
B -- files_to_process --> C{PrepareProcessingItemsStage};
|
||||
C -- processing_items (FileRule) --> D{PipelineOrchestrator};
|
||||
D -- FileRule --> E(RegularMapProcessorStage);
|
||||
E --> F{Check FileRule.item_type};
|
||||
F -- Starts with "MAP_"? --> G[Process Map];
|
||||
F -- No --> H[Skip Map / Log Warning];
|
||||
G --> I[...subsequent stages...];
|
||||
H --> I;
|
||||
@@ -1,72 +0,0 @@
|
||||
# Processing Pipeline Refactoring Plan
|
||||
|
||||
## 1. Problem Summary
|
||||
|
||||
The current processing pipeline, particularly the `IndividualMapProcessingStage`, exhibits maintainability challenges:
|
||||
|
||||
* **High Complexity:** The stage handles too many responsibilities (loading, merging, transformations, scaling, saving).
|
||||
* **Duplicated Logic:** Image transformations (Gloss-to-Rough, Normal Green Invert) are duplicated within the stage instead of relying solely on dedicated stages or being handled consistently.
|
||||
* **Tight Coupling:** Heavy reliance on the large, mutable `AssetProcessingContext` object creates implicit dependencies and makes isolated testing difficult.
|
||||
|
||||
## 2. Refactoring Goals
|
||||
|
||||
* Improve code readability and understanding.
|
||||
* Enhance maintainability by localizing changes and removing duplication.
|
||||
* Increase testability through smaller, focused components with clear interfaces.
|
||||
* Clarify data dependencies between pipeline stages.
|
||||
* Adhere more closely to the Single Responsibility Principle (SRP).
|
||||
|
||||
## 3. Proposed New Pipeline Stages
|
||||
|
||||
Replace the existing `IndividualMapProcessingStage` with the following sequence of smaller, focused stages, executed by the `PipelineOrchestrator` for each processing item:
|
||||
|
||||
1. **`PrepareProcessingItemsStage`:**
|
||||
* **Responsibility:** Identifies and lists all items (`FileRule`, `MergeTaskDefinition`) to be processed from the main context.
|
||||
* **Output:** Updates `context.processing_items`.
|
||||
|
||||
2. **`RegularMapProcessorStage`:** (Handles `FileRule` items)
|
||||
* **Responsibility:** Loads source image, determines internal map type (with suffix), applies relevant transformations (Gloss-to-Rough, Normal Green Invert), determines original metadata.
|
||||
* **Output:** `ProcessedRegularMapData` object containing transformed image data and metadata.
|
||||
|
||||
3. **`MergedTaskProcessorStage`:** (Handles `MergeTaskDefinition` items)
|
||||
* **Responsibility:** Loads input images, applies transformations to inputs, handles fallbacks/resizing, performs merge operation.
|
||||
* **Output:** `ProcessedMergedMapData` object containing merged image data and metadata.
|
||||
|
||||
4. **`InitialScalingStage`:** (Optional)
|
||||
* **Responsibility:** Applies configured scaling (e.g., POT downscale) to the processed image data received from the previous stage.
|
||||
* **Output:** Scaled image data.
|
||||
|
||||
5. **`SaveVariantsStage`:**
|
||||
* **Responsibility:** Takes the final processed (and potentially scaled) image data and orchestrates saving variants using the `save_image_variants` utility.
|
||||
* **Output:** List of saved file details (`saved_files_details`).
|
||||
|
||||
## 4. Proposed Data Flow
|
||||
|
||||
* **Input/Output Objects:** Key stages (`RegularMapProcessor`, `MergedTaskProcessor`, `InitialScaling`, `SaveVariants`) will use specific Input and Output dataclasses for clearer interfaces.
|
||||
* **Orchestrator Role:** The `PipelineOrchestrator` manages the overall flow. It calls stages, passes necessary data (extracting image data references and metadata from previous stage outputs to create inputs for the next), receives output objects, and integrates final results (like saved file details) back into the main `AssetProcessingContext`.
|
||||
* **Image Data Handling:** Large image arrays (`np.ndarray`) are passed primarily via stage return values (Output objects) and used as inputs to subsequent stages, managed by the Orchestrator. They are not stored long-term in the main `AssetProcessingContext`.
|
||||
* **Main Context:** The `AssetProcessingContext` remains for overall state (rules, paths, configuration access, final status tracking) and potentially for simpler stages with minimal side effects.
|
||||
|
||||
## 5. Visualization (Conceptual)
|
||||
|
||||
```mermaid
|
||||
graph TD
|
||||
subgraph Proposed Pipeline Stages
|
||||
Start --> Prep[PrepareProcessingItemsStage]
|
||||
Prep --> ItemLoop{Loop per Item}
|
||||
ItemLoop -- FileRule --> RegProc[RegularMapProcessorStage]
|
||||
ItemLoop -- MergeTask --> MergeProc[MergedTaskProcessorStage]
|
||||
RegProc --> Scale(InitialScalingStage)
|
||||
MergeProc --> Scale
|
||||
Scale --> Save[SaveVariantsStage]
|
||||
Save --> UpdateContext[Update Main Context w/ Results]
|
||||
UpdateContext --> ItemLoop
|
||||
end
|
||||
```
|
||||
|
||||
## 6. Benefits
|
||||
|
||||
* Improved Readability & Understanding.
|
||||
* Enhanced Maintainability & Reduced Risk.
|
||||
* Better Testability.
|
||||
* Clearer Dependencies.
|
||||
Reference in New Issue
Block a user