-
Notifications
You must be signed in to change notification settings - Fork 128
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
MagicFlare refactors #292
base: master
Are you sure you want to change the base?
MagicFlare refactors #292
Conversation
73578ff
to
97060bb
Compare
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Removed.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Removed.
src/graphics/particle/MagicFlare.cpp
Outdated
void MagicFlareContainer::removeAll() { | ||
|
||
for(size_t i = 0; i < g_magicFlaresMax; i++) { | ||
MagicFlare& flare = g_magicFlares[i]; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 80d6021.
src/graphics/particle/MagicFlare.cpp
Outdated
RenderMaterial mat; | ||
mat.setBlendType(RenderMaterial::Additive); | ||
|
||
EERIE_LIGHT* light = lightHandleGet(torchLightHandle); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 80d6021.
src/graphics/particle/MagicFlare.cpp
Outdated
@@ -451,7 +451,7 @@ void MagicFlareReleaseEntity(const Entity * entity) { | |||
g_magicFlares.removeEntityPtrFromFlares(entity); | |||
} | |||
|
|||
long MagicFlareCountNonFlagged() { | |||
long MagicFlareCountWithoutIO() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/graphics/particle/MagicFlare.cpp
Outdated
if(flare.exist) { | ||
removeFlare(flare); | ||
} | ||
removeFlare(flare); |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
src/graphics/particle/MagicFlare.cpp
Outdated
, size(0.f) | ||
, io(nullptr) | ||
, bDrawBitmap(false) | ||
{ } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Removed.
Sorry this took me a few months to get back to. All of the pointed out issues should be sorted now. |
391b7f7
to
3d8a7e6
Compare
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.