Post Port: Code review, refactor about nifskope HOT 4 CLOSED

(Tip: You can append ?w=1 to the url to remove purely whitespace changes)

Qt Support

Disabling deprecated behavior with macros turns them into compiler errors so that they can be found and replaced. Changes were minimal. First commit involved just using the method Qt already had to return char pointers instead of using a cast. Second commit outright disabled anything deprecated as of 5.2, and the only thing that needed to be changed were the use of Qt algorithms that were deprecated in favor of the standard library.

Build/project

Header/Includes

Whitespace

First commit is manual changes I made for the most noticeable indentation issues I happened across. Some were using 3 spaces per indent, some were just a mix of tabs and spaces.

Second commit is automated changes using Uncrustify**. Before running it I spent literally days tweaking the settings and verifying everything went over well. In preparation I had to add brackets to foreach macros because it's not standard C++. It was tripping up on a few files where for example there would be a bracketed if statement inside of a braceless foreach. I don't blame it, since it's a terrible idea to nest something bracketed under something not. Anyway, I did manage to miss a few of these before running it, but the only thing it did was mess up the indentation level on one file (just had to Shift-Tab the code back to the first column). There is virtually no chance that it did anything to affect the integrity of the code, as I set it to make whitespace changes only. There are optional settings to auto-bracket single-line statements (or the opposite) and make if/else chains use consistent bracketing, but I made such changes manually in later commits.

**: I will eventually finish the code style guide and also upload the Uncrustify config file I used.

Code Style (Formatting only)

Aside from the addition of brackets, these changes were mostly aesthetic. The addition of brackets was to prevent future issues and improve the overall integrity of the code. There were some highly questionable if/else chains with a mix of brackets and no brackets. There were single-line if statements separated by comments, or bracketed statements underneath braceless statements. There were multi-nested single line statements, which is just as bad. There was one actual completely braceless if/else chain with one level of nesting (if ( if / else ) else ). There were if/else chains that were all kept on one line per statement (no indentation).

Code Style

I removed many "if return / else return" idioms. If a scope is guaranteed to return there is no need to nest an else/else if underneath it.

I additionally inverted conditionals where possible to reduce nesting. Take for example:

// Unnecessary nesting

if ( true ) {
    if ( !somethingElse ) {
        // Do this
        doThis();
        doThisOtherThing();

        return true;
    } else {
        return false;
    }
} else {
    return false;
}

// Instead

if ( false )
    return false;

if ( somethingElse )
    return false;

// Do this
doThis();
doThisOtherThing();

return true;

Fixes

These undid a few changes that I made that upon review actually changed the result. I've reviewed the changes multiple times now.

C++11

from nifskope.

I noticed that QRegExp is being superseded by QRegularExpression starting in Qt 5 and I started replacing it where it was used. But I've run into some issues. For one, I'm not sure it worked before. The one place I've dealt with anyway, which is the node culling.

For example if I enter in (ignoring the pipe separators) NiTriShape ... It does not cull NiTriShapes. If I keep with the style and use the ^ i.e. ^NiTriShape it doesn't do anything either. I have tried (non-exhaustively):

Ni
^Ni
Ni*
^Ni*
NiTriShape
^NiTriShape
^NiTriShape$
NiTriShapeData

And so on. And yes, Cull Node by Name is turned on.

In fact, there is another issue going on here. The edit box should be greyed out if it is not checked, but all of my builds post-Qt5 seem to ignore it.** Furthermore it's very slow to type in those fields in my builds, but the old releases are faster.

**It must be an intentional change in 1.1.3 or earlier. I use 1.1.0 as a backup because opening 1.1.3 constantly resets the UI while using dev versions, and the box is greyed out in 1.1.0. Additionally it's commented out in code:

        CullExpr->setToolTip( tr( "Enter a regular expression. Nodes which names match the expression will be hidden" ) );
//    leave RegExp input open as both rendering and collada cull sharing this
//    CullExpr->setEnabled( CullByID->isChecked() );
        CullExpr->setEnabled( true );
        connect( CullExpr, SIGNAL( textChanged( const QString & ) ), this, SIGNAL( sigChanged() ) );
//    connect( CullByID, SIGNAL( toggled( bool ) ), CullExpr, SLOT( setEnabled( bool ) ) );

Ohhhhh. I see the issue here. :) I am considering the node name to be "NiTriShape" etc, when it's really the Name value on the block. Guess that is something to add perhaps.

That still leaves the slowness with that text edit and maybe others. I will look into all the other text edit fields.

from nifskope.

OK, I narrowed down the slowness to nothing but having Textures enabled (or loaded) across all versions. Even 1.1.0 has sluggish UI elements once I added my texture paths back (<1.1.4 can't autodetect the Skyrim BSAs). When you disable textures, the textbox for Cull Nodes by Name becomes "fast" again.

The presence of textures also makes the other UI elements sluggish, like the Ambient/Diffuse/Specular and Background/Foreground color wheels.

So maybe I can make improvements there so that these elements are more responsive when textures are being rendered.

    connect( Options::get(), SIGNAL( sigChanged() ), textures, SLOT( flush() ) );
    connect( Options::get(), SIGNAL( sigChanged() ), this, SLOT( update() ) );

Narrowed it down to this in glview.cpp (old signal syntax).

Basically, sigChanged might be too vague a signal. Since almost all the options do not need a texture flush.

Fix

Selectively flush textures for large speed up 5487659

from nifskope.

Updated top comment with all basic code refactoring commits.

Deferred Tasks

• Clean up comments (formatting, spelling, grammar).

from nifskope.