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

MagicFlare refactors #292

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

MagicFlare refactors #292

wants to merge 122 commits into from

Conversation

bsxf-47
Copy link
Contributor

@bsxf-47 bsxf-47 commented Oct 5, 2023 •
edited
Loading

Attempted to put the whole module into a more readable shape. Kept the C style array for speed and the header functions so that there isn't yet another global object floating about.

@bsxf-47 bsxf-47 force-pushed the interactive branch 5 times, most recently from 73578ff to 97060bb Compare October 7, 2023 16:56
@bsxf-47 bsxf-47 marked this pull request as ready for review October 7, 2023 21:27
dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/graphics/particle/MagicFlare.cpp Outdated
@@ -502,7 +502,7 @@ void FlareLine(Vec2f fromPos, Vec2f toPos, Entity * io) {
long z = Random::get(0, FLARELINERND);
z += FLARELINESTEP;
if(!io) {
z = long(z * g_sizeRatio.y);
z *= long(g_sizeRatio.y);
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong, long(g_sizeRatio.y) is going to change abruptly from 1 (height 480 - 959) to 2 (height 960 to 1439) to 3 (1440p+) etc. instead of scaling the y offset smoothly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. Removed.

dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/graphics/particle/MagicFlare.cpp Outdated
@@ -522,7 +522,7 @@ void FlareLine(Vec2f fromPos, Vec2f toPos, Entity * io) {
long z = Random::get(0, FLARELINERND);
z += FLARELINESTEP;
if(!io) {
z = long(z * g_sizeRatio.y);
z *= long(g_sizeRatio.y);
Copy link
Member

Choose a reason for hiding this comment

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

Same, I don't think the change of rounding from after to before the multiplication is wanted 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.

My bad. Removed.

dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/graphics/particle/MagicFlare.cpp Outdated
void MagicFlareContainer::removeAll() {

for(size_t i = 0; i < g_magicFlaresMax; i++) {
MagicFlare& flare = g_magicFlares[i];
Copy link
Member

@dscharrer dscharrer Oct 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This should be MagicFlare & flare

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 80d6021.

dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/graphics/particle/MagicFlare.cpp Outdated
RenderMaterial mat;
mat.setBlendType(RenderMaterial::Additive);

EERIE_LIGHT* light = lightHandleGet(torchLightHandle);
Copy link
Member

Choose a reason for hiding this comment

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

This should be EERIE_LIGHT * light

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 80d6021.

dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/graphics/particle/MagicFlare.cpp Outdated
@@ -451,7 +451,7 @@ void MagicFlareReleaseEntity(const Entity * entity) {
g_magicFlares.removeEntityPtrFromFlares(entity);
}

long MagicFlareCountNonFlagged() {
long MagicFlareCountWithoutIO() {
Copy link
Member

Choose a reason for hiding this comment

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

IO → Entity would be even better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed.

dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/graphics/particle/MagicFlare.cpp Outdated
@@ -283,7 +283,7 @@ long MagicFlareContainer::countWithoutIO() {

long count = 0;
for(size_t i = 0; i < m_magicFlaresMax; i++) {
if(g_magicFlares[i].exist && !g_magicFlares[i].hasIO) {
if(g_magicFlares[i].exist && !g_magicFlares[i].io) {
Copy link
Member

Choose a reason for hiding this comment

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

Intentional? .io is cleared when the entity is destructed, .hasIO is not. Not sure if this actually matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know it was like this in the original code, but I removed member variable altogether in a later commit.

dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/scene/Light.cpp Outdated
@@ -297,7 +297,9 @@ EERIE_LIGHT * lightHandleGet(LightHandle lightHandle) {
void lightHandleDestroy(LightHandle & handle) {

if(EERIE_LIGHT * light = lightHandleGet(handle)) {
light->m_exists = false;
if(light) {
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant, the first if already checks the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably a brain fart. Removed.

dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/graphics/particle/MagicFlare.cpp Outdated
if(flare.exist) {
removeFlare(flare);
}
removeFlare(flare);
Copy link
Member

Choose a reason for hiding this comment

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

Worried that his might still call io->flarecount-- for already-cleared flares. Perhaps the io member should also be reset in MagicFlare::clear()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flare is made to completely reset in a later commit.

dscharrer
dscharrer reviewed Oct 28, 2023
View reviewed changes
src/graphics/particle/MagicFlare.cpp Outdated
, size(0.f)
, io(nullptr)
, bDrawBitmap(false)
{ }
Copy link
Member

Choose a reason for hiding this comment

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

These are redundant if already initialized to the same values in-line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Removed.

bsxf-47 added 22 commits January 28, 2024 15:05
@bsxf-47
MagicFlare: Rename member function
c3d1389
@bsxf-47
MagicFlare: Rename member variable
95a7d7a
@bsxf-47
MagicFlare: Use member variable
33ab1e2
@bsxf-47
MagicFlare: Remove extraneous parameter
ea54c2f
@bsxf-47
MagicFlare: Remove extraneous parameter
13c30a8
@bsxf-47
MagicFlare: Use member variable
8adccd3
@bsxf-47
MagicFlare: Remove extraneous parameter
ed26163
@bsxf-47
MagicFlare: Use wrapper function
4625215
@bsxf-47
MagicFlare: Use wrapper function
c6202e2
@bsxf-47
MagicFlare: Use wrapper function
12c5ed3
@bsxf-47
MagicFlare: Use Vec2f
43c9f6e
@bsxf-47
MagicFlare: Use Vec2f
1712bf2
@bsxf-47
MagicFlare: Use local variables
c13c337
@bsxf-47
MagicFlare: Use local variables
b90e79b
@bsxf-47
MagicFlare: Remove redundant check
0284750
@bsxf-47
MagicFlare: Rewrite function to remove duplicate code
70f175e
@bsxf-47
MagicFlare: Remove helper function
bc53268
@bsxf-47
MagicFlare: Extract member function
96ef76f
@bsxf-47
MagicFlare: Pass positions as const references
903c15b
@bsxf-47
MagicFlare: Remove redundant member variable
de938f5
@bsxf-47
MagicFlare: Fix code style
80d6021
@bsxf-47
MagicFlare: Mark constructor as explicit
8811a16
@bsxf-47
Copy link
Contributor Author

bsxf-47 commented Jan 28, 2024

Sorry this took me a few months to get back to. All of the pointed out issues should be sorted now.

@bsxf-47 bsxf-47 requested a review from dscharrer February 15, 2024 21:05
@dscharrer dscharrer force-pushed the master branch 5 times, most recently from 391b7f7 to 3d8a7e6 Compare May 30, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dscharrer dscharrer Awaiting requested review from dscharrer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bsxf-47 @dscharrer