Skip to content

Review #1

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

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

Review #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
adbf8f2
first comment
Solnec Oct 12, 2022
0cb7152
comments
Solnec Oct 13, 2022
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
7 changes: 4 additions & 3 deletions Globals.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,7 +1441,7 @@ bool SingletonGlobalVars::LockFromExecuting(ModuleAddresses* modules, bool& isAd
//-------------------------------------------------------------//
void SingletonGlobalVars::NewRound(AddressesList& addressesList, bool& newRound)
{
int newMatch = *(int*)(addressesList.inMatch);
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Почему бы не хранить сразу int* в AddressesList, все равно каждый раз приводишь к int* и точно ли есть необходимость в арифметике указателей?

int newMatch = *(int*)(addressesList.inMatch);//
if (!(newRound) && newMatch)
{
newRound = true;
Expand All @@ -1451,8 +1451,9 @@ void SingletonGlobalVars::NewRound(AddressesList& addressesList, bool& newRound)
}

//-------------------------------------------------------------//
void SingletonGlobalVars::FindMaxId(const int maxPlayers, AddressesList& addressesList, int maxId)
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Вроде бы нет необходимости в out параметре обычное нахождение максимума

unsigned int SingletonGlobalVars::FindMaxId(const int maxPlayers, AddressesList& addressesList)
{
unsigned int maxId = 0;
for (int i = 0; i < maxPlayers; i++)
{
int bufferId = *(int*)(addressesList.entBase + i * addressesList.playerOffset);
Expand Down Expand Up @@ -1480,7 +1481,7 @@ void SingletonGlobalVars::FillPlayerData(const int& maxPlayers, AddressesList& a

//names attacker
if (ents.size())
for (std::shared_ptr<Ent>& player : ents)
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

в range based циклах можно опускать полное имя типа для простоты

for (auto& player : ents)
{
//fill names
if (allPlayerNames.size())
Expand Down
20 changes: 10 additions & 10 deletions aim.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@
#include "ent.h"
#include "aim.h"
#include "memory.h"
#include <numbers>

constexpr float kModelHeight = 64.118f;
constexpr float kHeadAdjustment = 1.9f;
constexpr float kCrouchAdjustment = 3.0f;
constexpr float kOnGround = 2.0f;
constexpr float kCrouchHeightAdjustment = -35.0f;
constexpr float kDegreesToRad = PI / 180.0f;
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Можно использовать стандартные средства языка

constexpr float kRadToDegrees = 180.0f/ PI;
constexpr float kDegreesToRad = std::numbers::pi_v<float> / 180.0f;
constexpr float kRadToDegrees = 180.0f/ std::numbers::pi_v<float>;

constexpr float kDefaultDistAdjustment = 920.0;
constexpr float kDistanceMultiplier = 900.0f;
Expand All @@ -35,17 +36,16 @@ int iterator = 0;

bool Aim::IsOnGround(int stance) const
{
bool isFallen = false;
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Ненужная переменная, просто возвращай true/false

if (stance < 50
|| stance == 1234
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Magic numbers лучше чтобы все константы были названы. Например помести эти числа в enum

|| stance == 1257
|| stance == 1256
|| stance == 1258)
{
isFallen = true;
return true;
}

return isFallen;
return false;
}

//-------------------------------------------------------------//
Expand Down Expand Up @@ -322,12 +322,12 @@ float Aim::AngleBetweenEnemyX(float X, float Y) const
if (((X < 0) && (Y > 0)) || ((X < 0) && (Y < 0)))
{
angle = atan(Y / X);
angle += (float)PI;
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

С-style cast почти всегда плохая идея, он незаметный в коде и может ломать код. Если очень надо, то лучше использовать reinterpret_cast но обычно такая необходимость значит, что есть ошибка в архитектуре

angle += std::numbers::pi_v<float>;
}
else if ((X > 0) && (Y < 0))
angle = (float)(2 * PI + atan(Y / X));
angle = 2 * std::numbers::pi_v<float> + atan(Y / X);
else
angle = (float)atan(Y / X);
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

тут atan итак возвращает float

angle = atan(Y / X);

float finalAngle = (float)((kRadToDegrees) * angle);
return finalAngle;
Expand Down Expand Up @@ -608,7 +608,7 @@ std::shared_ptr<Ent> Aim::FindClosestToCrosshair(std::vector<std::shared_ptr<Ent
}

//-------------------------------------------------------------//
void Aim::Pblocks(std::shared_ptr<Ent> &enemy)
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Передавать умный указатель по ссылке нужно только осознавая, что в таком случае счетчик ссылок может обнулиться в неподходящий момент, и ссылка перестанет ссылаться на валидные данные. В данном случае вроде бы можно, но тогда по константной ссылке, все равно только чтение

void Aim::Pblocks(std::shared_ptr<Ent> &enemy)//
{
Weapons weapons;
const float pbPitch = 25;
Expand Down Expand Up @@ -721,7 +721,7 @@ void Aim::Pblocks(std::shared_ptr<Ent> &enemy)
}

//-------------------------------------------------------------//
std::shared_ptr<Ent> Aim::ChooseTarget(std::shared_ptr<Ent> &chosenTarget, const bool &isTk, bool &isAim)
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

зачем константная ссылка на бул? Дешевле скопировать 1 байт чем разрядность_системы_бит. Обычно бул передают по значению

std::shared_ptr<Ent> Aim::ChooseTarget(std::shared_ptr<Ent> &chosenTarget, const bool isTk, bool &isAim)
{
std::vector<std::shared_ptr<Ent>> bufferEnts;
std::vector<std::shared_ptr<Ent>> bufferEntsVis;
Expand Down
2 changes: 1 addition & 1 deletion aim.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class Aim


//choose target
std::shared_ptr<Ent> ChooseTarget(std::shared_ptr<Ent> &chosenTarget, const bool &isTk, bool& isAim);
std::shared_ptr<Ent> ChooseTarget(std::shared_ptr<Ent> &chosenTarget, const bool isTk, bool& isAim);


//lock on
Expand Down
8 changes: 5 additions & 3 deletions ent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,15 +162,15 @@ struct PlayerNames

///////////////////////////////////////////////////////////////
// class Ent - entity (player)
class Ent
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Названия классов лучше не сокращать, улучшается читабельность

class Ent//Entity
{
public:
int color1;
int color2;
int id;
int surface;
int saberAnim;
int numWeapon;
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Количество оружия не может быть отрицательным -> лучше использовать беззнаковый инт. Индексы массива тоже всегда неотрицательны(но тут принято size_t) и т.д. если можно использовать более "узкий" тип данных лучше использовать его, поможет избежать случайных ошибок и повысит читабельность

unsigned int numWeapon;
int stance;
int stance2;
int hp;
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

В производном классе не нужен член myHp, зачем дублировать уже унаследованный челн класса?
Слишком много полей в одном классе, слишком много на себя берет, нужно разбивать на иерархию скорее всего или на отдельные модули которые будут взаимодействовать между собой.

Expand Down Expand Up @@ -204,8 +204,10 @@ class Ent
bool correctColor3;
bool correctColor4;

const char* className;
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Зачем дублировать имя?

//name.c_str()

std::string name;

std::string shortName;

Info2d info2d;
Expand Down
3 changes: 2 additions & 1 deletion nothingSuspicious.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
<LinkIncremental>true</LinkIncremental>
<OutDir>C:\Users\ооррр\Desktop\epicHax</OutDir>
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Пути в проекте лучше делать относительными, а не абсолютными, например у меня такой папки не было на компьютере

<OutDir>.\x64</OutDir>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
<LinkIncremental>false</LinkIncremental>
Expand Down Expand Up @@ -127,6 +127,7 @@
<ConformanceMode>true</ConformanceMode>
<PrecompiledHeader>Use</PrecompiledHeader>
<PrecompiledHeaderFile>pch.h</PrecompiledHeaderFile>
<LanguageStandard>stdcpp20</LanguageStandard>
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Вроде в readme было написано что 20ый стандарт используется, а по факту стоял 14ый(дефолтьный)

</ClCompile>
<Link>
<SubSystem>Windows</SubSystem>
Expand Down
7 changes: 4 additions & 3 deletions vec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "pch.h"
#include "vec.h"

#include <numbers>


float DotProduct(const vec3_t vec1, const vec3_t vec2)
Expand All @@ -25,15 +26,15 @@ void VecSub(vec3_t in, vec3_t added)
}

//-------------------------------------------------------------//
void VectorSubtract(const vec3_t vec1, const vec3_t vec2, vec3_t vecOut)
void VectorSubtract(const vec3_t& vec1, const vec3_t& vec2, vec3_t& vecOut)
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

1.Выходной параметр обязательно должен передаваться по ссылке, иначе создается копия, которая в последствии и изменяется внутри метода и при выходе из скоупа удаляется
2.vec1 и vec2 тоже желательно передавать по константной ссылке(констаность была правильной уже), а по ссылке чтобы не копировать

{
vecOut[0] = vec1[0] - vec2[0];
vecOut[1] = vec1[1] - vec2[1];
vecOut[2] = vec1[2] - vec2[2];
}

//-------------------------------------------------------------//
void VecDivByNum(vec3_t in, float numDiv)
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Лучше назвать более нейтрально так как по сути это in_out параметр и по ссылке конечно

void VecDivByNum(vec3_t& in, float numDiv)
{
in[0] = in[0] / numDiv;
in[1] = in[1] / numDiv;
Expand Down Expand Up @@ -103,7 +104,7 @@ void AngleVectors(float yaw, float pitch, float roll, vec3_t forward, vec3_t rig
static float sr, sp, sy, cr, cp, cy;
// static to help MS compiler fp bugs

float angleDegrees = (float)(PI * 2 / 360);
float angleDegrees = std::numbers::pi_v<float> * 2.0 / 360.0;

angle = yaw * angleDegrees;
sy = sinf(angle);
Expand Down
8 changes: 2 additions & 6 deletions vec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
#pragma once
#include <cmath>
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

В header-ы лучше не подключать лишних библиотек или заголовочных файлов, потому что ты их потом везде подключаешь и это раздувает объектные файлы и порождает циклические зависимости.


constexpr float PI = 3.14159265358979323846;

typedef float vec3_t[3];
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

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

Вместо множества свободных функций лучше сделать полноценный класс 3д вектора с переопределенными операторами т .д. . В коде это будет выглядеть как v1 - v2 и т.д.
Как минимум поместить в отдельный namespace т.к. засорять глобальную область видимости лучше не надо

typedef float vec2_t[2];

Expand All @@ -17,10 +13,10 @@ void VecAdd(vec3_t in, vec3_t added);
void VecSub(vec3_t in, vec3_t added);

//VectorSubstract - substract vectors end result into vecOut
void VectorSubtract(const vec3_t vec1, const vec3_t vec2, vec3_t vecOut);
void VectorSubtract(const vec3_t& vec1, const vec3_t& vec2, vec3_t& vecOut);

//VecDivByNum divide vector by number
void VecDivByNum(vec3_t in, float numDiv);
void VecDivByNum(vec3_t& in, float numDiv);

//VecNull - make a vector 0,0,0
void VecNull(vec3_t in);
Expand Down