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

jfmherokiller · 2023-10-23T14:25:56Z

feel free to poke around here. this draft is more an experiment then an actual draft atm.

jfmherokiller · 2023-10-23T22:46:15Z

using static constexpr seems to incrase compile time but atleast under my tests also provides some marginal speedups

Sources/Plasma/FeatureLib/pfDXPipeline/plDXEnumerate.h

Sources/Plasma/Apps/plClient/win32/plClient_Win.cpp

Co-authored-by: Michael Hansen <zrax0111@gmail.com>

Sources/Plasma/PubUtilLib/plMath/plTriUtils.cpp

colincornaby · 2023-10-25T05:01:13Z

Sources/Plasma/PubUtilLib/plMath/plTriUtils.cpp

+ 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
+ };


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

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

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.

@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.

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.

@dgelessus Thanks! I'll review the docs.

colincornaby · 2023-10-25T05:10:56Z

Sources/Plasma/FeatureLib/pfDXPipeline/plDXTextFont.cpp

 | 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,


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.

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.

i can revert this if desired

Hoikas · 2023-10-26T23:19:28Z

You may revert or remove 843c93b

jfmherokiller · 2023-10-31T00:26:47Z

dropped the commit

Hoikas

This PR covers an odd set of topics. I think we would prefer to see changesets grouped by topic where possible.

Sources/Plasma/Apps/plPythonPack/main.cpp

Sources/Plasma/PubUtilLib/plMath/plTriUtils.cpp

Hoikas · 2023-11-03T00:43:02Z

Sources/Tools/plFilePatcher/main.cpp

@@ -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)


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?

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

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

shouldnt HS_BUILD_FOR_WIN32 handle that?

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

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

Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>

Hoikas · 2023-11-10T18:11:30Z

Due to the changes to pfDXPipeline, this will be deferred until after #1326 is merged.

jfmherokiller · 2023-11-12T12:13:21Z

fine by me I just hope the mac port lands successfully

jfmherokiller added 5 commits October 23, 2023 07:17

deleting a null pointer has no effect

258c692

make these values constexpr

3e4201e

use getfiletype instead of _isatty on windows

e7858e3

set the identity matrix to constexpr

0d1ce28

use range based for loop instead

e53e7ec

Deledrius added the hacktoberfest-accepted label Oct 23, 2023

use static constexpr

83e11f2

zrax reviewed Oct 23, 2023

View reviewed changes

Sources/Plasma/FeatureLib/pfDXPipeline/plDXEnumerate.h Outdated Show resolved Hide resolved

Sources/Plasma/Apps/plClient/win32/plClient_Win.cpp Outdated Show resolved Hide resolved

jfmherokiller and others added 6 commits October 23, 2023 18:25

use std::size instead of begin and end

4ea1271

Co-authored-by: Michael Hansen <zrax0111@gmail.com>

add const to the for loop

d5b0ce6

Co-authored-by: Michael Hansen <zrax0111@gmail.com>

change kFVF to constexpr

9840aea

use enhanced for loop

19d1149

use enhanced for loop

79bd917

set these variables to constexpr

628841a

jfmherokiller marked this pull request as ready for review October 25, 2023 03:58

colincornaby reviewed Oct 25, 2023

View reviewed changes

Sources/Plasma/PubUtilLib/plMath/plTriUtils.cpp Show resolved Hide resolved

colincornaby reviewed Oct 25, 2023

View reviewed changes

jfmherokiller force-pushed the other-cleanups branch from 843c93b to 628841a Compare October 31, 2023 00:26

Hoikas reviewed Nov 3, 2023

View reviewed changes

jfmherokiller and others added 5 commits November 3, 2023 12:49

defer copy

d9d3b8e

Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>

use emplace_back instead

da62cf3

Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>

fix indentation

9ef170e

revert constexpr

080ae43

handle mingw

1684bb1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jfmherokiller commented Oct 23, 2023 •

edited

Loading

jfmherokiller commented Oct 23, 2023

colincornaby Oct 25, 2023

jfmherokiller Oct 25, 2023

colincornaby Oct 25, 2023

colincornaby Oct 26, 2023

dgelessus Oct 26, 2023

colincornaby Oct 27, 2023

colincornaby Oct 25, 2023

Hoikas Oct 26, 2023

jfmherokiller Oct 31, 2023

Hoikas commented Oct 26, 2023

jfmherokiller commented Oct 31, 2023 •

edited

Loading

Hoikas left a comment

Hoikas Nov 3, 2023

jfmherokiller Nov 3, 2023

Hoikas Nov 5, 2023

jfmherokiller Nov 5, 2023

dpogue Nov 5, 2023

Hoikas Nov 10, 2023 •

edited

Loading

Hoikas commented Nov 10, 2023

jfmherokiller commented Nov 12, 2023

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

Are you sure you want to change the base?

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

Conversation

jfmherokiller commented Oct 23, 2023 • edited Loading

jfmherokiller commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoikas commented Oct 26, 2023

jfmherokiller commented Oct 31, 2023 • edited Loading

Hoikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoikas Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Hoikas commented Nov 10, 2023

jfmherokiller commented Nov 12, 2023

jfmherokiller commented Oct 23, 2023 •

edited

Loading

jfmherokiller commented Oct 31, 2023 •

edited

Loading

Hoikas Nov 10, 2023 •

edited

Loading