Refactoring jamboree

[Nintynuts]

After getting code compiling on VS2019, I have further tidied up references and got other projects building. This required reconstructing "IDirect3DUnknown.h".

Beyond this, I have also renamed the main classes in DX9 and DX10 because I didn't like the use of namespaces to differentiate classes with the same name, so made them consistent with DX11 by calling them Hacker*.

Finally, the main point of all this was that DX10 and DX11 are now sharing commmon code when it comes to Shader information caching.

This pull request isn't quite ready, needs testing and reviewing, potentially some more refactoring.

Yes, I am aware there is probably too much stuff in this PR, but I kept finding things I wanted to fix while stuck on other issues, and it was too tricky to pick apart the specific refactors I intended to do at the start from the bulk fixes I did in the process.

[DarkStarSword]

As mentioned on Discord, most of these changes will go away once this is rebased on the vs2017 branch. Retargetting to a new toolchain and/or SDK becomes a trivial operation in this branch that can be done via the Visual Studio (Project -> Retarget Solution) or even a search and replace over the project files. I did try vs2019 at one point in the past, and hit some compile errors that will need to be fixed up. We will need to decide whether we want to jump straight to vs2019, or relegate that to a topic branch as vs2015/2017 have been for years. I don't anticipate there being as many issues migrating to vs2019 as there were migrating to vs2015, however I do anticipate there to be other unexpected issues as no migration has ever gone entirely smoothly in my experience.

The DX9 project should not be touched at the moment, and tread carefully around common files (i.e. those outside the DirectX11 folder), pending the merge of Dave's DX9 port currently in the dx9 topic branch. If you do wish to touch common files, it might be worthwhile nagging me to merge the dx9 branch asap - I've already sorted out the issues in this branch that broke our core DX11 build (and most of those fixes are already in master), and I'll just need to double check if I had any other pending changes to make it work in conjunction with vs2017. That all said, the DX9 branch has some crash issues due to some fairly major architectural problems and is not really ready for public release - but provided it doesn't break DX11 there's no harm in bringing the code into master (but we may need to disable it in "Zip Release" until it's ready).

The DX10 project is bit-rotted and inactive, and as such I have no objections to any changes you want to make in it - Bo3b did revive this and did an experimental DX10 build some years back, but it hasn't been touched since then. However, if you aren't actually planning on making a DX10 build there's no reason to touch this. Ideally any sort of work in this project would be towards the goal of splitting DX version specific code from other 3DMigoto code and part of a broader objective to make 3DMigoto run on DX9 + DX12. Make sure you are using the "Zip Release" build configuration in the solution as it excludes inactive projects like DX10, whereas "Release" builds most projects in Release configuration, including those that are not active at the moment.

Most of the D3DCompiler and DXGI projects are in similar states to DX10, however some of these are semi-active and used to act as a loader for 3DMigoto in very special cases. For core development you shouldn't need to touch these, and even if you do need to use them - the releases of these we have already made should continue to work.

You might want to invite https://github.com/bo3b as another reviewer.

[DarkStarSword]

The BinaryDecompiler project is a C++ port of https://github.com/James-Jones/HLSLCrossCompiler (with some work from other forks merged in).

Please refrain from making any unecessary changes in this third party code - we should be able to diff our code with upstream and see exactly what we have changed without being distracted by superfluous changes. As it is I am considering reverting Chiri's changes to port it to C++ and just treat it as a C module as that would be a lot easier to diff to upstream, and a lot easier to merge in updates.

Edit: Keeping these files as close as possible to upstream also trumps cleaning up any coding style or whitespace issues in these files btw - those type of changes would ideally be submitted upstream first and then we update to include them, however upstream does not have an active maintainer at present so there is no one to accept them. I keep an eye on any new forks of the upstream project to check if there are any bug fixes we might want, hence why a smaller diff makes my life easier.

[DarkStarSword]

Why is this moved to Globals? Assigned shaders are per-context and definitely not global.

[DarkStarSword]

Unecessary coding style changes.

[DarkStarSword]

A general comment before I go further - I'm seeing too many changes in this commit. Refactoring should be separate from migrating to a new compiler.

[DarkStarSword]

This was broken up on separate lines to avoid the need to scroll horizontally.
See this:

I can read it.

See this:

I can't read that.

[DarkStarSword]

There's a couple of other places you removed or added a "..". I'll never object to these being removed, but if you're adding one it probably indicates that the include path isn't set correctly for the project and that should be fixed instead.

[DarkStarSword]

As stated above, shaders are assigned to per-context in DirectX11 (In DirectX10 they are per-device), so these absolutely should not be moved out of here - this will break games using multi-threaded rendering pipelines.

[DarkStarSword]

This is a coding style change, and debatable whether it is better or worse. "auto" saves a lot of keystrokes, but can mask bugs where an incorrect type has been used. I won't object to new code using "auto", but there needs to be a good reason for altering already written and well tested code.

[DarkStarSword]

We don't have an official coding style for 3DMigoto, so for your own code you are fairly free to use whatever you prefer, provided that it is readable and doesn't mask bugs. There might be a few things here or there we pick up on, but until we adopt an actual coding style, please consider them suggestions rather than rules. However, if you are working in existing code, please refrain from changing the coding style unecessarily and try to adopt whatever is already there*

This discussion isn't one way - if you feel strongly that a particular coding style is good/bad/ugly you can bring it up for discussion. If we were to adopt a coding style, Bo3b has suggested Google's coding style guidelines (but again, no decision has been made to actually do so): http://google.github.io/styleguide/

Any functions you see that declare all their variables at the top like this one are most likely written by myself following the coding styles of the Linux Kernel, which make functions more readable than declaring variables inline. This is definitely not a coding style we are considering as a whole for 3DMigoto, as it is designed for a C project, and some of the guidelines do not work well in C++, but (like all coding style guidelines) it does make some good points.

* One exception is Chiri's old code, which both Bo3b and I agree has some... serious problems. If you see any large (and I mean freaking huge) functions, that's a leftover from Chiri and those are candidates for refactoring and splitting up into multiple smaller more understandable functions... but they are also going to be the hardest and riskiest parts of the code to actually do that refactoring on and should not be taken on lightly.

[DarkStarSword]

using namespace std; is considered bad practice, and should especially be avoided in header files. I know we have used it in a few other places in 3DMigoto, but if you don't need it, don't add it. Certainly don't add it here.

https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice

[DarkStarSword]

I'm not certain I like these typedefs masquerading as defines. Looks dangerous, please justify.

[DarkStarSword]

Why is this being moved into the DX11 project? IIDs are independent of what DirectX version we are building for. In fact, in the future this should look like this to get the IIDs of all our HackerDevice versions regardless of which version we are currently building for (alternatively all our IID definitions accross all projects could be moved to a single common file):

#include "DirectX9/HackerDevice.h"
#include "DirectX10/HackerDevice.h"
#include "DirectX11/HackerDevice.h"
#include "DirectX11/HackerContext.h"
#include "DirectX11/FrameAnalysis.h"

[DarkStarSword]

TLS is not specific to locking (actually the main use here is for hooking_quirk_protection), so it doesn't really belong in a file called lock.h. It's not exactly big enough that it specifically needs to be in its own file on that grounds alone, but if you do need to use outside of the DX11 project it might be better splitting it into its own file.

[DarkStarSword]

I'm sensing there might have been some circular include conflicts to require this header reorganisation? We should perhaps consider moving to a model where header files prefer to use forward declarations over including other headers where possible, and the cpp files include the headers they need, as that can help a lot to avoid the circular include dependency hell. For now - if this works it's fine, but some food for thought for the future.

[DarkStarSword]

I had gone out of my way to make this util.h work for DX9+DX11 and as much as possible I don't want yet more copies of the same code (we already have too much of that with DX10 being a copy+paste of DX11 and Dave's DX9 port being the same), so please move it back.

Edit: Looking further through the review it looks like you split it up into multiple smaller files? That's probably a good thing, but because the move and splitting was done in a single commit I can't easily see how util.h itself was changed for the review process.

[DarkStarSword]

As mentioned earlier, all changes to the DX9 project are rejected pending the merge of Dave's DX9 port. That said, the DX9 port does require some work to bring it more inline with the DX11 architecture, so some of these changes might well be wanted at a later date.

[DarkStarSword]

See earlier comment - adding ".." means the project include paths are set wrong.

[DarkStarSword]

Pretty sure I dropped that one intentionally (IIRC it never did anything). Please don't bring it back. I'd actually like to deprecate & drop all these HLSL autofixes, but I'm not entirely certain if anyone has used them and they usually stay out of the way.

[DarkStarSword]

IDirect3DUnknown is from Chiri's original code ( 1ba6f7e#diff-95006bfff5a5f88ff0f40f730d65afaaR5 ), and Bo3b long since got rid of this while re-architecting 3DMigoto from the ground up, so I don't think we want to bring this back.

If you got any references to this it will be in bit-rotted projects that hadn't been re-architected yet and you shouldn't be building if using the "Zip Release" configuration.

[DarkStarSword]

I dunno whether this should really be called nvapi.cpp, or just our_big_fat_hack.cpp. It's not really part of nvapi, so using that name may be misleading, but it does interact with our nvapi wrapper... I dunno, you can call it whatever you like I suppose.

I'd actually rather change our big fat hack to just use an exported DLL function with a name that like 'migoto_override()' rather than overloading nvapi's QueryInterface API, and to remove the potential for race conditions by making the override and the following API call one and the same, but this works as is and that kind of change is for another day.

[DarkStarSword]

nvapi.h is a third party header from nvidia and should not be altered. NvAPIOverride is our own big fat hack and should go in our own headers.

[DarkStarSword]

I can't add a comment on nvstereo.h since it's just a rename, but I wanted to point out that there are at least a couple of resources on the web that point to our version of this file and their links would break if this were moved. That's probably not a big deal, since our version isn't "canonical", and theoretically other devs should be pulling it from nvidia and not us, but the actual canonical version is a) very hard or impossible to find and b) not ported to DX11.

Also, this file and a bunch of the other common files are used by the DX9 port, so that will need to be adjusted accordingly.

[DarkStarSword]

Ok, I've finished my first pass looking over everything. For the most part this looks fine, however the migration to vs2019 and the refactor work absolutely needs to be split up into separate commits, because it is not clear if we will accept the vs2019 port into master or if that will be relagated to a topic branch that runs alongside master.

The refactoring work should also be broken up into smaller commits (e.g. breaking util.h up into separate files could be a commit all to itself, as could moving things to HackerCommon, as could renaming certain variables, as could coding style changes, etc), and some of that refactor work is going to be impacted by the DX9 port and I'd strongly suggest holding off on any major refactoring until that has been merged.

As mentioned elsewhere, moving our tracking of assigned shaders out of the context into globals must be reverted, as that will break multi-threaded rendering.

As a general comment - while refactoring is generally a good thing to make the code more manageable, it inevitably introduces regressions no matter how careful you are. Well tested but ugly code can be better than clean newly refactored code. We do have plenty of code in 3DMigoto that needs to be refactored (some desperately), but the benefits of refactoring old code needs to be weighed up with the potential for introcuding regressions. As it is with the DX9 port, the switch to VS2017 (or 2019), the switch to d3dcompiler_47 and the switch to the new Windows SDK the next release of 3DMigoto is going to be a boat rocker and I am anticipating regressions - so, please don't do any unecessary refactoring work at this stage that risks making it even more shaky.

[DarkStarSword]

This is probably ok, though I'd need to double check if it's safe to use a reference here - certainly if you don't know you need to change this, you probably shouldn't.

That aside, there is another slight issue with the coding style here, and that's the placement of the ampersand next to the type name rather than the variable name. In this particular context it doesn't matter too much, but I do have a preference on putting the * or & next to the variable name everwhere (other than return types or situations where the variable name is otherwise omitted), because of this ambiguity:

int* a, b, c;

Did I just declare three pointers, or a pointer and two ints?

Based on the placement of the * it looks like I declared three pointers, but I actually declared one pointer and two ints. Declaring it as such:

int *a, b, c;

Is slightly clearer as to how the language interprets it. You can argue that this is a very stupid part of C/C++ (and you would be absolutely right) and insist that the * is part of the type so it should be on the left, but it is what it is and I've seen this bite programmers before hence my preference to put it on the right with the variable name. (and int * a, b, c; is not a compromise but rather the worst of both worlds).

As mentioned elsewhere we haven't adopted a coding style for 3DMigoto, so take this as a suggestion with justification rather than a rule. I won't block new code that goes against this suggestion if you do prefer it the other way, but I would prefer that existing code not be altered.

[DarkStarSword]

Noticing your commit message:

Commit messages should be a short title, followed by a blank line, then a description. Please read the best practices on writing commit messages for a git project, and you can look through 3DMigoto's commit history for plenty of examples: https://chris.beams.io/posts/git-commit/

[DarkStarSword]

BTW, I've been considering renaming "Zip Release" back to "Release" and either dropping the current "Release" configuration or renaming it to something else. The reasoning is pretty simple: you aren't the first person to check out the code and run into issues compiling inactive projects that have long since bit-rotted. I'd also like to make the DX11 project the default for new users (not being able to set this outside of a per-user file is a long standing issue in Visual Studio, but I believe it can be worked around by manually editing the sln file to put the desired project first in the list), so that someone can download the code and have it build first time.

[Nintynuts]

Well done for looking through it all. I definitely got a bit carried away tidying stuff up while figuring out problems, and I totally agree it would be easier/better in separate commits. I think I will scrap this branch and PR, start fresh from the vs2017 branch and do it in smaller chunks.

Regarding coding style, any changes I've made (barring lines being a bit long) are definitely my preference (which I feel quite strongly about) and particulary the positions of pointer and reference I feel should be with the type and list declaration is a quirk of the language where it confuses the point, where I would just not use it.

With the several files you think contain inappropriate content I did it because it seemed related by usage. Where files shouldn't be changed at all its just that I didn't know. Either way, that's fine I'll just have to split them out. Id recommend we mark files somehow to say do not change if thats the case (maybe put the in a separate folder or project?)

[DarkStarSword]

Regarding coding style, any changes I've made (barring lines being a bit long) are definitely my preference (which I feel quite strongly about) and particulary the positions of pointer and reference I feel should be with the type and list declaration is a quirk of the language where it confuses the point, where I would just not use it.

That's fine - understanding and knowing to look out for the potential pitfalls is more important than the actual placement; rather my preference is to use the placement to signify the potential pitfall to new coders.

With the several files you think contain inappropriate content I did it because it seemed related by usage. Where files shouldn't be changed at all its just that I didn't know. Either way, that's fine I'll just have to split them out. Id recommend we mark files somehow to say do not change if thats the case (maybe put the in a separate folder or project?)

Yeah, that's not a bad idea. UE4 puts third party code in a "ThirdParty" folder for much this reason.