ousnius / nifly

C++ NIF library for the Gamebryo/NetImmerse File Format
GNU General Public License v3.0
54 stars 21 forks source link

BGSM/BGEM support #37

Closed SK83RJOSH closed 2 years ago

SK83RJOSH commented 2 years ago

Pull request for tracking BGSM/BGEM support. Looking for feedback, and some ideas for how this could be better integrated into nifs themselves.

SK83RJOSH commented 2 years ago

Okey-dokey, I would say this review is in a semi-ready state. Something's remain to be done, such as tests, variable naming, and some general clean up. I won't take it any further until I can get some feedback, though. 🙂

ousnius commented 2 years ago

@SK83RJOSH Material files aren't something that I think should be in nifly. nifly is a library for NIF files only. BGSM files are something else entirely and external, plus limited to Bethesda games. It's a pity they decided to not properly use texture sets inside the NIF file anymore for FO76, but that means another library has to deal with that, not nifly.

My other repository for Outfit Studio already has C++ code for BGSM/BGEM files and Material Editor does so as well in C#. Feel free to make another new repo for a modern C++ BGSM/BGEM library (if such doesn't exist already by someone else) and use my other two repos as reference (they both already support FO76 files).

ousnius commented 2 years ago

I saw that you added code to save out material files alongside the NIF as well. This doesn't make much sense as the game reuses the same material files for multiple NIF files (that's their entire point).

SK83RJOSH commented 2 years ago

Material files aren't something that I think should be in nifly.

Fair enough, this was something I asked about before starting, as I assumed it may not be a good fit.

Plus limited to Bethesda games

I feel this point is a bit moot with the focus of this repository being BGS titles almost exclusively. However, that doesn't negate the fact that indeed it feels a bit out of scope.

The issue I have ultimately is that the library has many utility functions for textures and shading that do not work. Which as has been demonstrated, is confusing for the end user if they don't already know about materials, nevermind that they will have to reimplement these functions themselves.

Is there any way to improve that particular pain point I wonder?

My other repository for Outfit Studio already has C++ code for BGSM/BGEM files

Wasn't aware of this, that would've been handy last week. ☚ī¸

(if such doesn't exist already by someone else)

Seems doubtful. The fact this library exists is already rather surprising. 🙂

This doesn't make much sense as the game reuses the same material files for multiple NIF files (that's their entire point).

The idea was to support the user making modifications to a material through nifly and saving that. Of course I know the point is material reuse – but in practice they very rarely reuse materials and almost always create one per object. Regardless, it's merely a convenience function ultimately.

ousnius commented 2 years ago

@SK83RJOSH

The issue I have ultimately is that the library has many utility functions for textures and shading that do not work. Which as has been demonstrated, is confusing for the end user if they don't already know about materials, nevermind that they will have to reimplement these functions themselves.

Is there any way to improve that particular pain point I wonder?

The material/texture information isn't in the NIF file, so nifly can't retrieve it. Any tool using nifly (such as Outfit Studio, PyNifly, ...) needs to make use of another library to read the external material file and retrieve the shader/texture properties from it, if it wants to support that.

It's up to the tool to then fill the properties that nifly comes with in its NIF shader classes or do it separately, though it would make sense to fill nifly's properties with the data from the material file, which is what Outfit Studio does for the most part.

SK83RJOSH commented 2 years ago

@ousnius aha, I see, I suppose that's an acceptable solution. Though I'm curious, would it make sense to refactor the shader part of the code to support external user data in some way?

My thoughts are separating the shader getters/setters into a separate interface, and the blocks themselves only deal with the raw data, and you implement the interface separately:

class ShaderProperties : public UserDataBase {
   virtual ShaderType GetType() const virtual = 0;
}

class NifShaderProperties final : public ShaderProperties {
    NifBlock* block = nullptr;
    ShaderProperties(NifBlock* b) : block(b) { }
    ~ShaderProperties() final = default;
    ShaderType GetType() const final { return block->type; }
}

class NifBlock final : public NifReversible<NifBlock, NifObject> {
    ShaderType type;
    NifBlock() final {
        SetUserData<ShaderProperties>(std::make_unique<NifShaderProperties>(this));
    }
    // Get/Put/Sync
}

void main() {
    // contrived example
    auto nif = nifly::new();
    nif.root.SetUserData<ShaderProperties>(std::make_unique<BmShaderProperties>(bm::new()));
    const auto* props = nif.root.GetUserData<ShaderProperties>();
}

Where user data is implemented as a std::unordered_map<std::type_index, std::unique_ptr<UserDataBase>> or similar?

This way it's trivial for blocks to be augmented through external code easily, and users can attach user data of their own to each block for their own purposes if need be. Feels like a nice middle ground, but totally understand if I'm overthinking this. 🙂