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

Feature #3523: Light source on magic projectiles #1144

Merged
8 commits merged into from Dec 4, 2016
Merged

Feature #3523: Light source on magic projectiles #1144

8 commits merged into from Dec 4, 2016

Conversation

Aussiemon
Copy link
Contributor

@Aussiemon Aussiemon commented Dec 4, 2016

Feature #3523

Attached light to magic bolt projectiles.

Thread discussion of this pull request

@@ -2,6 +2,8 @@

#include <iomanip>

#include <osg/Light>
#include <osg/LightSource>
Copy link

Choose a reason for hiding this comment

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

Correct include is components/sceneutil/lightmanager.hpp

@@ -136,7 +138,7 @@ namespace MWWorld
};


void ProjectileManager::createModel(State &state, const std::string &model, const osg::Vec3f& pos, const osg::Quat& orient, bool rotate, std::string texture)
void ProjectileManager::createModel(State &state, const std::string &model, const osg::Vec3f& pos, const osg::Quat& orient, bool rotate, bool isMagic, std::string texture)
Copy link

Choose a reason for hiding this comment

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

isMagic should be renamed to createLight to reflect the variable's real purpose.

@Aussiemon Aussiemon changed the title [Incomplete] Feature #3523: Light source on magic projectiles Feature #3523: Light source on magic projectiles Dec 4, 2016
@Aussiemon
Copy link
Contributor Author

Retrieval of colors from the relevant magic effects is complete. Testing on several spells shows that the blended diffusion values match the projectile colors in vanilla Morrowind.

@ghost
Copy link

ghost commented Dec 4, 2016

Retrieval of colors from the relevant magic effects is complete. Testing on several spells shows that the blended diffusion values match the projectile colors in vanilla Morrowind.

That sounds very.. scientific ;)

@ghost ghost merged commit 4269a2d into OpenMW:master Dec 4, 2016
@MiroslavR
Copy link
Collaborator

@scrawl, can we agree not to use C-style casts anymore?

@ghost
Copy link

ghost commented Dec 7, 2016

Oh, I didn't notice the casts.

A static_cast<float> would be preferred over (float), but these are a very minor issue that I'm not going to bother bringing up in review.

Regarding this cast:

int numberOfEffects = ((MagicBoltState&)state).mEffects.mList.size();

Is a problem, the function will crash if for some reason state.mIdMagic has a value with a non-magic projectile.

@Aussiemon: could you make a new PR updating the code to not use C-style casts, and remove the MagicBoltState cast? It's probably easiest to just compute the light color before hand and then pass it to createModel.

@MiroslavR
Copy link
Collaborator

A static_cast would be preferred over (float), but these are a very minor issue that I'm not going to bother bringing up in review.

I was primarily referring to non-trivial types. The ability to fall back to reinterpret_cast sounds like an accident waiting to happen. C++ casts express their intent much more clearly.

@ghost
Copy link

ghost commented Dec 7, 2016

Yes, that is my stance. For non-trivial types, we should always use C++ casts.

In this case, the cast shouldn't be there in the first place though.

@MiroslavR
Copy link
Collaborator

In this case, the cast shouldn't be there in the first place though.

Agreed.

@Aussiemon
Copy link
Contributor Author

Thanks for the review of my work. I'll definitely take this into account in the future.

I've created a pull request with the recommended changes to casts. If there's a better way to make these fixes, I'd be happy to learn.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants