Refactor courtesy @gavrant

[hexabits]

No description provided.

[gavrant]

Hi! Glad that my changes have been merged, I'm all for it, was going to create a PR myself when I'm done. But there's one problem: it happened way too early. Must admit, the merged commits were still somewhat WIP and not polished for public release. For example, by now I know that I've made a few mistakes in them. And I'm currently in the middle of upgrading Mesh-Shape classes (focuses: significanly improved vertex/triangle selection, some bug fixes in nif data updates, better performance for Skyrim SE and newer meshes). And also halfway through that upgrade it finally hit me how to make all the NifModel getting/setting/checking code (the bulk of NifModel - NifItem - QModelIndex stuff) far easier to write and read. In other words, a huge "refactoring of the refactored" incoming.

[hexabits]

@gavrant
Is anything inherently broken? NifSkope 2.0 has always been "Dev" pre-alpha releases, i.e. use at your own risk. I reviewed most of your changes and didn't have any objections, only glowing praise.

Also I will be merging lots of things from other branches but I have to do most of it manually now since your refactor was so significant (though very welcome). So maybe keep an eye on pushes because you may start having merge conflicts with my branch.

[gavrant]

Nothing seriously broken, it's mostly stuff like a typo in an error message, BaseModel::getItem not passing reportErrors arg downstream, or set<int>( header, "BS Header\\BS Version", 0 ) in NifModel::loadHeader doing nothing because of the set<int>( header, "User Version", 0 ) right above.

At the moment, the biggest issue for me is that I'm somewhat at a crossroads of what to do with my "how to make all the NifModel getting/setting/checking code far easier to write and read" idea. Here's an example of working with NifModel items to illustrate the idea:

void Foo( const NifModel * nif, const QModelIndex & index, int link )
{
    // Get first (0) child of "Children" subitem of index, then get its value as link and convert it to a NifItem.
    // With the old code it would be:
    // auto block1 = nif->getBlockItem( nif->getLink( nif->getItem( index, "Children" ), 0 ) );
    auto block1 = ( *nif >> index >> "Children" >> 0 ).getLinkBlock();

    // Same as above, but report the error if it fails to get "Children" or 0 subitems.
    // Also check if the result link block inherits BSTriShape.
    auto block2 = ( *nif >> index ).child( "Children" ).child( 0 ).getLinkBlock( "BSTriShape" );

    // Resolve link as a NifItem, get its "Name" subitem, get its value as QString
    QString someStr = ( *nif >> link >> "Name" ).get<QString>();
    
    if ( block1.inherits( "BSTriShape" ) ) {
        // Get tangent value of each vertex in "Vertex Data"
        for ( auto vertex : ( block1 >> "Vertex Data" ).childIterator() )
            Vector3 t = ( vertex >> "Tangent" ).get<Vector3>();
    }
    
    ( block1 >> "Name" ).set<QString>( "Hello" ); // Compile error because nif is a const pointer
}

Note that the code does not use NifItem pointers directly, it's all actually objects of a container class for NifItem * instead, to make it more in the "functional programming" style.

Pros: in my opinion, this makes the code shorter and cleaner, easier to read and write. It would also allow to ditch tons of overloads for methods in NifModel class.

Cons: without a rewrite of all the existing code (spells, nodes, etc.) it's somewhat semi-useless. And not only a total rewrite would require a lot of time from me (I can afford that), but it will make merging code from other people very arduous, far more so than it is now with my previous refactor.

What's your opinion? Is this idea worthy of implementing and merging here, despite all the time and effort it would require from me and you? Maybe it's worthy, but without the total rewrite of spells, nodes, etc.?

[hexabits]

@gavrant

Oh also, btw, if you run File Checker and there are any errors, the new reportError stuff will create errors because the checker is threaded and the dialogs can't be created from another thread.

I'll read and respond to your new message after I'm done trying to debug these issues with updating NifSkope to the latest nif.xml.

[hexabits]

I'll read and respond to it even further in a bit, but initial thoughts, I really don't like the syntax.

Secondly, I think the most important thing about a major refactor would be moving away from naive strings as the field identifiers. It's extremely problematic when it comes to nifxml changing names and then NifSkope not knowing about it. The use of an incorrect field identifier should ideally be a compile error. I have some prototypes of how to approach this buried somewhere, maybe in my Gists, and I will dig those up sometime to show you.

@gavrant what is your Discord username? I think maybe we should chat on there instead. I can send you a friend requrest. Edit: Or you can just request me, mine is hexabit.

[gavrant]

I guess the dot in hexabit. is not actually a part of the Discord username? Just in case, my username is the same as here - gavrant.

[hexabits]

@gavrant no the dot is intentional. Someone managed to somehow take my name when discord switched to the new format... XD .. It says you're not accepting friend requests so you'll have to request me.