Improve CMakeList feature set and hide nvn impls to users

[ThePixelGamer]

This PR was mainly created to move the Impl headers out of the public folders as to keep their intended design, these headers are now being included into a new TU nvn_Impl.cpp which relies on compile definitions to choose which to include

While messing with the cmake system I went ahead and readjusted the VER macros a little to be more readable and any projects that use these changes will need to change their current version setup

set(NN_SDK_MAJOR 4)
set(NN_SDK_MINOR 4)
set(NN_SDK_PATCH 0)

set(NN_WARE_MAJOR 1)
set(NN_WARE_MINOR 6)
set(NN_WARE_PATCH 1)

to this

set(NN_SDK 4.4.0)
set(NN_WARE 1.6.1)

---

This change is 

[MonsterDruide1]

Reviewed 2 of 7 files at r1.
Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions (waiting on @leoetlino and @ThePixelGamer)

---

CMakeLists.txt line 18 at r1 (raw file):

if(NN_USE_GFX)
  if(NN_API STREQUAL "C")
    unset(NN_API)

Why do it like this? Wouldn't it be cleaner to keep this set like this, and from gfx_NvnHelper only include the Base instead of Impl?

---

src/NintendoSDK/nvn/nvn_Impl.cpp line 3 at r1 (raw file):

// included in gfx_NvnHelper.cpp
#ifndef NN_API_C
#include "nvn_FuncPtrImpl.h"

Are these still intended to be header files, or are those maybe just cpp files directly, which would also mean that they could be included/excluded in the CMake file instead?

[ThePixelGamer]

CMakeLists.txt line 18 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Why do it like this? Wouldn't it be cleaner to keep this set like this, and from gfx_NvnHelper only include the Base instead of Impl?

In order for nvnLoadCProcs to be in the correct position in the SMO binary nvn_FuncPtrImpl is required to be include by gfx_NvnHelper.cpp

[ThePixelGamer]

src/NintendoSDK/nvn/nvn_Impl.cpp line 3 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Are these still intended to be header files, or are those maybe just cpp files directly, which would also mean that they could be included/excluded in the CMake file instead?

I approached this as CmakeLists being optional and more so just setup

[MonsterDruide1]

Reviewed 3 of 7 files at r1.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @leoetlino and @ThePixelGamer)

---

src/NintendoSDK/nvn/nvn_Impl.cpp line 1 at r1 (raw file):

// included in gfx_NvnHelper.cpp

This comment is misleading

[ThePixelGamer]

src/NintendoSDK/nvn/nvn_Impl.cpp line 1 at r1 (raw file):

Previously, MonsterDruide1 wrote…

This comment is misleading

It got addressed in the latest commit

[MonsterDruide1]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @leoetlino)

---

src/NintendoSDK/nvn/nvn_Impl.cpp line 1 at r1 (raw file):

Previously, ThePixelGamer (ThePixelCoder) wrote…

It got addressed in the latest commit

True, I missed that.

[leoetlino]

Reviewed 5 of 7 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ThePixelGamer)

---

CMakeLists.txt line 5 at r3 (raw file):

project(NintendoSDK CXX ASM)

set(SDK_TARGET_PLATFORM "NVN" CACHE STRING "Set build type")

nit: maybe NNSDK_GFX_TARGET? IMO the name is a bit too generic right now

---

include/nn/util.h line 28 at r3 (raw file):

}  // namespace nn

#define NN_MAKE_VER(major, minor, patch) ((major << 16) | (minor << 8) | (patch))

please add parens around major, minor and patch (just to be safe)

---

include/nn/util.h line 41 at r3 (raw file):

#ifndef NN_SDK_TYPE
#define NN_SDK_TYPE "Release"

why was the ifndef removed?

[ThePixelGamer]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @leoetlino)

---

CMakeLists.txt line 5 at r3 (raw file):

Previously, leoetlino (Léo Lam) wrote…

nit: maybe NNSDK_GFX_TARGET? IMO the name is a bit too generic right now

I figured it would be better to leave it as a "global" define in case there's other areas in the SDK/Ware that have platform specific code (could also exclude the nvn API?)

---

include/nn/util.h line 28 at r3 (raw file):

Previously, leoetlino (Léo Lam) wrote…

please add parens around major, minor and patch (just to be safe)

To support if a user passes in a statement with another operator? I didn't initially think about that due to just using integral constants atm

---

include/nn/util.h line 41 at r3 (raw file):

Previously, leoetlino (Léo Lam) wrote…

why was the ifndef removed?

This was an oversight on my part while I was redesigning this section

[leoetlino]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ThePixelGamer)

---

CMakeLists.txt line 5 at r3 (raw file):

Previously, ThePixelGamer (ThePixelCoder) wrote…

I figured it would be better to leave it as a "global" define in case there's other areas in the SDK/Ware that have platform specific code (could also exclude the nvn API?)

I don't disagree with making this a global define, but what I'm saying is that the name is very generic and I think we should make it clearer that this is only for graphics. for me, I'd expect the SDK target platform to be Horizon or Win32 or something like that

[ThePixelGamer]

Reviewable status: 4 of 7 files reviewed, 3 unresolved discussions (waiting on @leoetlino and @MonsterDruide1)

---

CMakeLists.txt line 5 at r3 (raw file):

Previously, leoetlino (Léo Lam) wrote…

I don't disagree with making this a global define, but what I'm saying is that the name is very generic and I think we should make it clearer that this is only for graphics. for me, I'd expect the SDK target platform to be Horizon or Win32 or something like that

I adjusted the system differently based on this, I feel like my changes simplifies this system a bit more

[leoetlino]

let me re-review this tonight

[leoetlino]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ThePixelGamer)

---

CMakeLists.txt line 15 at r4 (raw file):

if(NN_GFX_TARGET)
  # gfx_NvnHelper.cpp includes the C impl
	if(NVN_API STREQUAL "C")

nit: indentation looks a bit wacky here (I think this is a tab?)

[leoetlino]

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @ThePixelGamer)

[ThePixelGamer]

Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @ThePixelGamer)

[ThePixelGamer]

Reviewed 4 of 7 files at r1, 2 of 3 files at r4, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ThePixelGamer)

[ThePixelGamer]

Reviewed 4 of 7 files at r1, 2 of 3 files at r4, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ThePixelGamer)