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.

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

New effect: Cocktail shaker #3569

Merged
merged 6 commits into from
Sep 30, 2023
Merged

Conversation

Veloscocity
Copy link
Contributor

Shakes all vehicles and props (not peds) back and forth, perpendicular to the direction the player is facing when the effect is triggered.

Copy link
Collaborator

@MoneyWasted MoneyWasted left a comment

Choose a reason for hiding this comment

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

Ensure that you are using clang formatting on MiscCocktail.cpp. The easiest way to achieve this in Visual Studio is by pressing CTRL+K followed by CTRL+D. This will format the current file. Otherwise, everything looks fine. I'll let Pongo do the final review.

ChaosMod/Effects/db/Misc/MiscCocktail.cpp Outdated Show resolved Hide resolved
ChaosMod/Effects/db/Misc/MiscCocktail.cpp Outdated Show resolved Hide resolved
ChaosMod/Effects/db/Misc/MiscCocktail.cpp Outdated Show resolved Hide resolved
Comment on lines 20 to 21
x = sin((360 - rot) * PI / 180) * 1.33;
y = -cos((360 - rot) * PI / 180) * 1.33;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
x = sin((360 - rot) * PI / 180) * 1.33;
y = -cos((360 - rot) * PI / 180) * 1.33;
x = sin((360 - rot) * PI / 180) * 1.33f;
y = -cos((360 - rot) * PI / 180) * 1.33f;

Don't think we need doubles here

Copy link
Collaborator

Choose a reason for hiding this comment

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

precomputing pi/180 would save some CPU cycles.

Copy link

@ShufflePerson ShufflePerson Sep 22, 2023

Choose a reason for hiding this comment

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

PI is defined in multiple other effects, could it be worth adding it to a common header file? Of course PI won't change, though it would still feel ""cleaner"" to have it defined in one place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe there is a DirectX header that includes all the common pi values such as pi/180, 180/pi, 2*pi, ect...

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a header file would be better than including the DirectX headers just when we need PI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's better to use SYSTEM::SIN and SYSTEM::COS natives: they work with angles in degrees, so you won't have to use the value of pi.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn’t it bet better to use the math functions from math.h, instead of calling it through game natives. Would definitely be faster.

Copy link

@ShufflePerson ShufflePerson Sep 22, 2023

Choose a reason for hiding this comment

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

cmath has all of that. But I don't see one for PI/180

#define _USE_MATH_DEFINES // for C++
#include <cmath>

#define _USE_MATH_DEFINES // for C
#include <math.h>

Value definitions: https://learn.microsoft.com/en-us/cpp/c-runtime-library/math-constants?view=msvc-170#remarks

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could (and probably should) just create our own header file that has definitions for all the possible numbers we could need and just include that file in stdafx.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so what am i supposed to do now

ConfigApp/Effects.cs Outdated Show resolved Hide resolved
@pongo1231
Copy link
Member

Looks good to me 👍

@pongo1231 pongo1231 merged commit 81857c5 into gta-chaos-mod:master Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants