Conversation
4b70ae6 to
6dff857
Compare
b8b13d7 to
a063a94
Compare
src/levelui.cpp
Outdated
| if (ImGui::MenuItem("Renderer")) { Windows::w_renderer = !Windows::w_renderer; } | ||
| if (ImGui::MenuItem("Ghosts")) { Windows::w_ghost = !Windows::w_ghost; } | ||
| if (ImGui::MenuItem("Python")) { Windows::w_python = !Windows::w_python; } | ||
| if (ImGui::MenuItem("Model Importer")) { m_showModelImporterWindow = !m_showModelImporterWindow; } |
There was a problem hiding this comment.
Save the variable in Windows:: data structure, as well as save its state to the settings file
There was a problem hiding this comment.
I see what you mean by the first suggestion (addressed), but I'm not sure what you mean by "as well as save its state to the settings file".
There was a problem hiding this comment.
Sorry, I wrote this message 4 days ago and the Windows:: structure doesn't exist anymore, so lets update this topic:
There's now a global Settings struct. As you can imagine, this is always loaded by the editor. ImGui saves its state globally as well. So, for window states, I'd like to always save the "latest" windows the user left open, so turning this bool into Settings::w_modelImporter and saving in the global settings file would be prefered.
| std::unordered_map<std::string, std::vector<uint8_t>> m_importedModels; | ||
|
|
||
| // Model textures placed in VRAM (filled by UpdateVRM, used by SaveLEV) | ||
| std::vector<ModelTextureForVRM> m_modelTexturesInVRAM; |
There was a problem hiding this comment.
Wouldn't it be better to unify these 3 structures to represent an instance? So the instance would be a name, its model texture and its definition (call it a class Instance). And then this cless has a serialize function and other helpers as needed.
There was a problem hiding this comment.
I'm not sure if this is as simple as you think it is,
- m_modelInstances : m_modelInstanceNames — 1:1 (parallel arrays, same index) - m_modelInstanceNames → m_importedModels — many:1 (multiple instances can reference the same model by name)
- m_importedModels → m_modelTexturesInVRAM — 1:many (each model can have multiple textures; each ModelTextureForVRM stores a modelName back-reference)
- m_modelInstances ↔ m_modelTexturesInVRAM — no direct relationship (both independently reference a model in m_importedModels)
There was a problem hiding this comment.
We'll need to call to discuss this further
| std::string m_log; | ||
| }; | ||
|
|
||
| namespace SH |
There was a problem hiding this comment.
SH? What does this name mean?
There was a problem hiding this comment.
Shared helpers. I didn't really care what it was called, I just wanted a short namespace so I could distinctly keep these separate from anything in psx_types
|
|
||
| // Extract raw PSX texture data (pixel indices + palette) without converting to RGBA | ||
| static void ExtractRawPSXTexture(const LevDataExtractor::VramBuffer& vram, | ||
| int pageX, int pageY, int palX, int palY, |
There was a problem hiding this comment.
Avoid this many function arguments. When you need too many arguments, it's a hint they should be a structure.
There was a problem hiding this comment.
I see what you mean, but I disagree in this case. The parameters of this function are
- Vram
- Page coordinate.
- texture metadata.
- a set of uv coords
- raw texture data (out)
It just so happens to be that describing some of these params (number 2, number 3, and number 4) are "verbose" and those parameters are "more than one parameter". I think making the args of this function a monolithic "argument structure" is silly and would have limited usage and would be more clutter than its worth. If you feel strongly about this I'll do it though.
There was a problem hiding this comment.
I agree that VRAM and textura data are clearly its own independent entities. Putting them together in a struct would suck as the function would be hard to re-use. As for addressing where a texture is in VRAM, you always need the texpage number, the palette, the bpp, the coordinates, so those can likely go in a struct (and there's likely many places in the editor that could use this struct). We might want to refactor some parts of the editor to use a common struct to refer to the texture in VRAM.
src/texture.h
Outdated
| // Model texture data for VRAM placement (from .ctrmodel) | ||
| struct ModelTextureForVRM | ||
| { | ||
| std::vector<uint8_t> pixelData; // Raw PSX format pixels |
There was a problem hiding this comment.
Thid structure seems to be repeated in the extractor file
There was a problem hiding this comment.
I tried to deduplicate the structs a bit, lmk what you think.
1ee206e to
9a99dd4
Compare
WIP