Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

this is a set of modifications to modernize parts of the code #1515

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

this is a set of modifications to modernize parts of the code #1515

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
258c692
deleting a null pointer has no effect
jfmherokiller Oct 23, 2023
3e4201e
make these values constexpr
jfmherokiller Oct 23, 2023
e7858e3
use getfiletype instead of _isatty on windows
jfmherokiller Oct 23, 2023
0d1ce28
set the identity matrix to constexpr
jfmherokiller Oct 23, 2023
e53e7ec
use range based for loop instead
jfmherokiller Oct 23, 2023
83e11f2
use static constexpr
jfmherokiller Oct 23, 2023
4ea1271
use std::size instead of begin and end
jfmherokiller Oct 23, 2023
d5b0ce6
add const to the for loop
jfmherokiller Oct 23, 2023
9840aea
change kFVF to constexpr
jfmherokiller Oct 23, 2023
19d1149
use enhanced for loop
jfmherokiller Oct 23, 2023
79bd917
use enhanced for loop
jfmherokiller Oct 24, 2023
628841a
set these variables to constexpr
jfmherokiller Oct 24, 2023
d9d3b8e
defer copy
jfmherokiller Nov 3, 2023
da62cf3
use emplace_back instead
jfmherokiller Nov 3, 2023
9ef170e
fix indentation
jfmherokiller Nov 3, 2023
080ae43
revert constexpr
jfmherokiller Oct 31, 2023
1684bb1
handle mingw
jfmherokiller Nov 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Sources/Plasma/Apps/plClient/win32/plClient_Win.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,9 @@ void plClient::InitDLLs()
{
hsStatusMessage("Init dlls client\n");
std::vector<plFileName> dlls = plFileSystem::ListDir("ModDLL", "*.dll");
for (auto iter = dlls.begin(); iter != dlls.end(); ++iter)
for (const auto& dll : dlls)
{
HMODULE hMod = LoadLibraryW(iter->WideString().data());
HMODULE hMod = LoadLibraryW(dll.WideString().data());
if (hMod)
{
pInitGlobalsFunc initGlobals = (pInitGlobalsFunc)GetProcAddress(hMod, "InitGlobals");
Expand Down
15 changes: 8 additions & 7 deletions Sources/Plasma/Apps/plPythonPack/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,18 +213,19 @@ void FindFiles(std::vector<plFileName> &filenames, std::vector<plFileName> &path
// Get the names of all the python files
std::vector<plFileName> pys = plFileSystem::ListDir(path, "*.py");

for (auto iter = pys.begin(); iter != pys.end(); ++iter)
for (const auto& py : pys)
{
filenames.push_back(iter->GetFileName());
filenames.push_back(py.GetFileName());
jfmherokiller marked this conversation as resolved.
Show resolved Hide resolved
pathnames.push_back(path);
}
}

void FindSubDirs(std::vector<plFileName> &dirnames, const plFileName &path)
{
std::vector<plFileName> subdirs = plFileSystem::ListSubdirs(path);
for (auto iter = subdirs.begin(); iter != subdirs.end(); ++iter) {
ST::string name = iter->GetFileName();
for (const auto& subdir : subdirs)
{
ST::string name = subdir.GetFileName();
jfmherokiller marked this conversation as resolved.
Show resolved Hide resolved
if (s_ignoreSubdirs.find(name) == s_ignoreSubdirs.end())
dirnames.push_back(name);
}
Expand All @@ -234,15 +235,15 @@ void FindPackages(std::vector<plFileName>& fileNames, std::vector<plFileName>& p
{
std::vector<plFileName> packages;
FindSubDirs(packages, path);
for (int i = 0; i < packages.size(); i++)
for (const auto& package : packages)
{
ST::string packageName;
if (!parent_package.empty())
packageName = parent_package + ".";
packageName += packages[i].AsString();
packageName += package.AsString();
std::vector<plFileName> packageFileNames;
std::vector<plFileName> packagePathNames;
plFileName packagePath = plFileName::Join(path, packages[i]);
plFileName packagePath = plFileName::Join(path, package);
FindFiles(packageFileNames, packagePathNames, packagePath);

// Check for the magic file to make sure this is really a package...
Expand Down
8 changes: 4 additions & 4 deletions Sources/Plasma/CoreLib/hsMatrix33.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,16 @@ hsMatrix33 operator*(const hsMatrix33& a, const hsMatrix33& b)

void hsMatrix33::Read(hsStream* s)
{
for (int i = 0; i < 3; i++)
for (auto& i : fMap)
{
s->ReadLEFloat(3, fMap[i]);
s->ReadLEFloat(3, i);
}
}

void hsMatrix33::Write(hsStream* s)
{
for (int i = 0; i < 3; i++)
for (const auto& i : fMap)
{
s->WriteLEFloat(3, fMap[i]);
s->WriteLEFloat(3, i);
}
}
2 changes: 1 addition & 1 deletion Sources/Plasma/FeatureLib/pfDXPipeline/plDXDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com
#define WEAK_ERROR_CHECK( cond ) cond
#endif

static D3DMATRIX d3dIdentityMatrix{
static constexpr D3DMATRIX d3dIdentityMatrix{
1.0f, 0.0f, 0.0f, 0.0f,
0.0f, 1.0f, 0.0f, 0.0f,
0.0f, 0.0f, 1.0f, 0.0f,
Expand Down
4 changes: 2 additions & 2 deletions Sources/Plasma/FeatureLib/pfDXPipeline/plDXDeviceRefs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ plDXTextureRef& plDXTextureRef::Set( D3DFORMAT ft, uint32_t ml, uint32_t mw, uin
fMaxHeight = mh;
fNumPix = np;
fDataSize = manSize;
if (fLevelSizes != nullptr)
delete [] fLevelSizes;

delete [] fLevelSizes;
if( lSz )
fLevelSizes = lSz;
else
Expand Down
10 changes: 0 additions & 10 deletions Sources/Plasma/FeatureLib/pfDXPipeline/plDXEnumerate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,6 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com

typedef LPDIRECT3D9 (WINAPI * Direct3DCreateProc)( UINT sdkVersion );

const uint8_t hsGDirect3DTnLEnumerate::kNumDisplayFormats = 6;
const D3DFORMAT hsGDirect3DTnLEnumerate::kDisplayFormats[] =
{
D3DFMT_A1R5G5B5,
D3DFMT_A2B10G10R10,
D3DFMT_A8R8G8B8,
D3DFMT_R5G6B5,
D3DFMT_X1R5G5B5,
D3DFMT_X8R8G8B8,
};

bool hsGDirect3DTnLEnumerate::SelectFromDevMode(const hsG3DDeviceRecord* devRec, const hsG3DDeviceMode* devMode)
{
Expand Down
13 changes: 10 additions & 3 deletions Sources/Plasma/FeatureLib/pfDXPipeline/plDXEnumerate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,16 @@ class hsGDirect3DTnLEnumerate
bool ICheckCubicRenderTargets( IDirect3D9 *pD3D, UINT iAdapter, D3DDEVTYPE deviceType, D3DEnum_ModeInfo *modeInfo );
HRESULT IConfirmDevice( D3DCAPS9* pCaps, DWORD dwBehavior, D3DFORMAT format );

static const uint8_t kNumDisplayFormats;
static const D3DFORMAT kDisplayFormats[];

static constexpr D3DFORMAT kDisplayFormats[] =
{
D3DFMT_A1R5G5B5,
D3DFMT_A2B10G10R10,
D3DFMT_A8R8G8B8,
D3DFMT_R5G6B5,
D3DFMT_X1R5G5B5,
D3DFMT_X8R8G8B8,
};
static constexpr uint8_t kNumDisplayFormats = std::size(kDisplayFormats);
public:
hsGDirect3DTnLEnumerate();
virtual ~hsGDirect3DTnLEnumerate();
Expand Down
2 changes: 1 addition & 1 deletion Sources/Plasma/FeatureLib/pfDXPipeline/plDXPipeline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ inline DWORD F2DW( FLOAT f )
#define WEAK_ERROR_CHECK( cond ) cond
#endif

static D3DMATRIX d3dIdentityMatrix{ 1.0f, 0.0f, 0.0f, 0.0f,
static constexpr D3DMATRIX d3dIdentityMatrix{ 1.0f, 0.0f, 0.0f, 0.0f,
0.0f, 1.0f, 0.0f, 0.0f,
0.0f, 0.0f, 1.0f, 0.0f,
0.0f, 0.0f, 0.0f, 1.0f };
Expand Down
7 changes: 2 additions & 5 deletions Sources/Plasma/FeatureLib/pfDXPipeline/plDXTextFont.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com

//// Local Stuff //////////////////////////////////////////////////////////////

static const long PLD3D_FONTFVF = D3DFVF_XYZ | D3DFVF_DIFFUSE
static constexpr long PLD3D_FONTFVF = D3DFVF_XYZ | D3DFVF_DIFFUSE
| D3DFVF_TEX1 | D3DFVF_TEXCOORDSIZE3(0);

static D3DMATRIX d3dIdentityMatrix{ 1.0f, 0.0f, 0.0f, 0.0f,
static constexpr D3DMATRIX d3dIdentityMatrix{ 1.0f, 0.0f, 0.0f, 0.0f,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if the identity matrix should be constexpr. Something being both static and constexpr may already be a bit of a paradox. But the identity matrix is loaded as a buffer from a pointer, which means it can't actually be constexpr easily by a compiler. And it might be more optimal just to have one copy of the buffer in memory anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static constexpr is perfectly valid. Sometimes you have things that can be used at both compile-time and runtime. In this case, we aren't doing any compile-time matrix math, so it probably makes more sense to just leave this one alone. I doubt that the generated machine code is any different since we're just going to be pushing a pointer onto the stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can revert this if desired

0.0f, 1.0f, 0.0f, 0.0f,
0.0f, 0.0f, 1.0f, 0.0f,
0.0f, 0.0f, 0.0f, 1.0f };
Expand All @@ -78,9 +78,6 @@ static D3DMATRIX d3dIdentityMatrix{ 1.0f, 0.0f, 0.0f, 0.0f,
//const uint32_t kNumVertsInBuffer(32768);
const uint32_t kNumVertsInBuffer(4608);

// See the declaration for plFontVertex in plTextFont.h for info
const DWORD plDXTextFont::kFVF = D3DFVF_XYZ | D3DFVF_DIFFUSE | D3DFVF_TEX1 | D3DFVF_TEXCOORDSIZE3(0);

IDirect3DVertexBuffer9* plDXTextFont::fBuffer = nullptr;
uint32_t plDXTextFont::fBufferCursor = 0;

Expand Down
3 changes: 2 additions & 1 deletion Sources/Plasma/FeatureLib/pfDXPipeline/plDXTextFont.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ class plDXTextFont : public plTextFont
void RestoreStates() override;
void DestroyObjects() override;

static const DWORD kFVF;
// See the declaration for plFontVertex in plTextFont.h for info
static constexpr DWORD kFVF = D3DFVF_XYZ | D3DFVF_DIFFUSE | D3DFVF_TEX1 | D3DFVF_TEXCOORDSIZE3(0);
};


Expand Down
17 changes: 9 additions & 8 deletions Sources/Plasma/PubUtilLib/plMath/plTriUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,18 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com
#include <cmath>
#include "plTriUtils.h"

static const float kAlmostZero = 1.e-5f;
static const float kPastZero = -kAlmostZero;
static const float kPastOne = 1.f + kAlmostZero;
static const float kAlmostOne = 1.f - kAlmostZero;
static const float kAlmostZeroSquared = kAlmostZero*kAlmostZero;
static constexpr float kAlmostZero = 1.e-5f;
static constexpr float kPastZero = -kAlmostZero;
static constexpr float kPastOne = 1.f + kAlmostZero;
static constexpr float kAlmostOne = 1.f - kAlmostZero;
static constexpr float kAlmostZeroSquared = kAlmostZero*kAlmostZero;
jfmherokiller marked this conversation as resolved.
Show resolved Hide resolved

static inline hsVector3 Cross(const hsScalarTriple& p0, const hsScalarTriple& p1)
{
return hsVector3(p0.fY * p1.fZ - p0.fZ * p1.fY,
p0.fZ * p1.fX - p0.fX * p1.fZ,
p0.fX * p1.fY - p0.fY * p1.fX);
return {p0.fY * p1.fZ - p0.fZ * p1.fY,
p0.fZ * p1.fX - p0.fX * p1.fZ,
p0.fX * p1.fY - p0.fY * p1.fX
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably remain an hsVector3 for type safety and clarity. I don't think there's any performance impact here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough i just found it as duplicating the type since the function already returns it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always better to use the defined constructor for an object instead of pushing in memory directly. Even if it's a bit duplicative with the type name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgelessus - I see your emoji. I'm poking at this and I see C++ is policing the number of elements in the struct. However - this struct does have a defined constructor and it does seem weird to go around it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I wanted to explain later and forgot, sorry. As I understand it, the brace syntax in this case will use the constructor defined in the class, despite the brace syntax. According to cppreference, the C-style field by field initialization is called aggregate initialization, which never applies to classes with user-defined constructors. Instead, the brace syntax does list initialization here, which in this case calls the normal constructor, exactly like the previous code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgelessus Thanks! I'll review the docs.

jfmherokiller marked this conversation as resolved.
Show resolved Hide resolved
}


Expand Down
8 changes: 4 additions & 4 deletions Sources/Tools/plFilePatcher/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com
#if HS_BUILD_FOR_WIN32
# include "hsWindows.h"
# include <io.h>
# define isatty _isatty
# define ttycheck(fileinfo) (GetFileType(fileinfo) == FILE_TYPE_CHAR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer to use standards compliant mechanisms. AFAIK, isatty is part of the POSIX standard. The Windows SDK just emits an erroneous deprecation warning with isatty and wants _isatty (which is just the Microsoft way of making life more difficult for cross-platform development). Is there a particular bug that is fixed by using (GetFileType(fileinfo) == FILE_TYPE_CHAR) instead of _isatty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it does if the user attempts compile and run the game using msys2 or cygwin the value of _isatty can be invalid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, you should probably fix the preprocessor checks to detect that situation to use isatty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt HS_BUILD_FOR_WIN32 handle that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HS_BUILD_FOR_WIN32 will be defined for MSVC, clang-cl, and mingw

In this case, you'll need an extra check for mingw to avoid aliasing isatty

Copy link
Member

@Hoikas Hoikas Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still seems to be using non-standards complaint naming, unfortunately. We prefer the POSIX standard isatty.


void GetConsoleWidth(size_t& width)
{
Expand All @@ -67,7 +67,7 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com
# include <sys/ioctl.h>
# include <sys/termios.h>
# include <unistd.h>

# define ttycheck(fileinfo) isatty(fileno(fileinfo))
void GetConsoleWidth(size_t& width)
{
struct winsize w;
Expand Down Expand Up @@ -174,7 +174,7 @@ int main(int argc, char* argv[])

patcher.SetPatcherFlags(flags);

if (!cmdParser.GetBool(kArgQuiet) && isatty(fileno(stdout))) {
if (!cmdParser.GetBool(kArgQuiet) && ttycheck(stdout)) {
patcher.SetDownloadBeginCallback(&FileDownloadBegin);
patcher.SetProgressCallback(&FileDownloadProgress);
}
Expand All @@ -185,7 +185,7 @@ int main(int argc, char* argv[])
return 1;
}

if (!cmdParser.GetBool(kArgQuiet) && isatty(fileno(stdout))) {
if (!cmdParser.GetBool(kArgQuiet) && ttycheck(stdout)) {
ST::printf("\r{}\r", ST::string::fill(consoleWidth, ' '));
fflush(stdout);
}
Expand Down
Loading