HomeHome  CalendarCalendar  FAQFAQ  SearchSearch  MemberlistMemberlist  UsergroupsUsergroups  RegisterRegister  Log inLog in  

Share | 
 

 Here are some fixes for some issues in the code.

View previous topic View next topic Go down 
AuthorMessage
eelke



Messages : 3
Date d'inscription : 2011-02-09

PostSubject: Here are some fixes for some issues in the code.   Thu Mar 10, 2011 6:34 am

Hi,

First first of all, thanks for creating this library. We've found it to be very useful.

I've found some issue while working with the code. I would like to share our fixes for them. Hopefully this is the right place for that.


*BUGS*


  1. There is a bug in QuadRendererInterface::computeAtlasCoordinates.
    textureIndex / textureAtlasNbY) / textureAtlasNbY should be textureIndex / textureAtlasNbX) / textureAtlasNbY.
    Notice that the first "textureAtlasNbY" is changed to textureAtlasNbX.
    (This bug would trigger when textureAtlasNbY != textureAtlasNbX, so for non-square atlas.)

  2. There was a crash bug in Interpolator::interpolate, but I see this is already fixed.
    Other than the crash bug I though it would be better to not go through the iterator each time you need the begin x and end x.

    So this:
    Code:

    // Else finds the current X in the range
    float newX = (currentKey.x - graph.begin()->x) / (graph.rbegin()->x - graph.begin()->x);
    newX -= static_cast<int>(newX);
    if (newX < 0.0f)
       newX = 1.0f + newX;
    currentKey.x = graph.begin()->x + newX * (graph.rbegin()->x - graph.begin()->x);
    Becomes:
    Code:

    // Else finds the current X in the range
    const float beginX = graph.begin()->x;
    const float rangeX = graph.rbegin()->x - beginX;
    float newX = (currentKey.x - beginX) / rangeX;
    newX -= static_cast<int>(newX);
    if (newX < 0.0f)
       newX = 1.0f + newX;
    currentKey.x = beginX + newX * rangeX;


  3. SPK.h is missing "#include "Extensions/Renderers/SPK_QuadRendererInterface.h"".


  4. Using an uninitialized member.
    Destroyer::Destroyer calls setTrigger with the uninitialized trigger member because its second parameter is unnamed.
    Code:

    Destroyer::Destroyer(Zone* zone,ModifierTrigger) :
       Modifier(INSIDE_ZONE | OUTSIDE_ZONE | INTERSECT_ZONE | ENTER_ZONE | EXIT_ZONE,INSIDE_ZONE,true,false,zone)
    {
       setTrigger(trigger);
    }
    should be:
    Code:

    Destroyer::Destroyer(Zone* p_zone,ModifierTrigger p_trigger) :
       Modifier(INSIDE_ZONE | OUTSIDE_ZONE | INTERSECT_ZONE | ENTER_ZONE | EXIT_ZONE,INSIDE_ZONE,true,false,p_zone)
    {
       setTrigger(p_trigger);
    }

  5. Missing virtual destructor for BufferCreator.
    Add a protected "virtual ~BufferCreator() {}" to BufferCreator class. I would also make the createBuffer function protected so derived classes can implement it.

  6. Registerable's copyChildren should not be virtual.
    Within the hierarchy the type of the first parameter changes and with it the signature. This means it's no longer the same function and a new virtual function is created instead of a new implementation for the existing one. The only reason this works is because of overloading, not virtual functions. The fix is to remove the virtual keyword for all copyChildren functions.

  7. Missing std namespace when calling std functions.
    You're including the C++ version of the C libraries which place the C functions in the std namespace, so you need to add the namespace when calling them. (Please do not fix by adding using namespace std.)

    • In the function "Transformable::setTransform" "memcpy" should be "std::memcpy".
    • In the function "Vector3D::getNorm()" "sqrt" should be "std::sqrt".
    • In the function "Oriented2DRendererInterface::rotateAndScaleQuadVectors" "cos" and "sin" should be "std::cos" and "std::sin".
    • In the function "Oriented3DRendererInterface::rotateAndScaleQuadVectors" "cos" and "sin" should be "std::cos" and "std::sin".
    • In the function "SphericEmitter::setAngles" "cos" and "sin" should be "std::cos" and "std::sin".
    • In the function "SphericEmitter::generateVelocity" "acos", "sin" and "cos" should be "std::acos", "std::sin" and "std::cos".



*WARNINGS*

We run our compilers on stricter warning settings than normal, we also have 'warning is error' turned on. Some might think that fixing these warnings is silly, but I've found some of the bugs in the previous list because the compiler warns about them.


  1. The last value of all the enums has a ',' behind it. This should not be there.

  2. Unused parameters. There are a lot of unused parameters.

  3. In the function "size_t Model::getParameterOffset(ModelParam param) const" the return type isn't the same as the type which is returned.
    (particleEnableIndices is an Int array, but size_t is the returntype.)

  4. Constant value in while.
    In the function "Group::sortParticles", "while (true)" is used. This causes a warning on higher warning levels. This can be fixed by using "for ( ;; )" while keeping the same behavior.

  5. Shadowing.
    There's a lot of variable shadowing. It's mostly parameters which have the same name as members. In some functions "this->" needs to be added in front of a variable name to distinguish a member from a parameter. Though this still creates warnings because the shadowing is still there. The way we fix this is to prefix parameters with "p_".

  6. The initialization order for some classes is incorrect.
    e.g. Emitter::Emitter's "active" variable should be initialized after "full".

  7. Missing forward declaration of class Group in SPK_Particle.h.
    When including only SPK_Particle.h the 'friend class Group' part won't compile because class Group is unknown. This can be fixed by adding "class Group; // forward declaration" right after "namespace SPK {".

Back to top Go down
View user profile
Juff
Developer


Messages : 539
Date d'inscription : 2009-07-14
Age : 34

PostSubject: Re: Here are some fixes for some issues in the code.   Thu Mar 10, 2011 7:42 am

Hi,

Thanks a lot for your report.
I'll take a closer look to all the bugs exposed and get back to you.
I ll release a new version with the fixes when I have time.
Back to top Go down
View user profile http://spark.developpez.com
eelke



Messages : 3
Date d'inscription : 2011-02-09

PostSubject: Re: Here are some fixes for some issues in the code.   Thu Mar 10, 2011 8:01 am

Awesome.

Let me know if you have any questions.
Back to top Go down
View user profile
Juff
Developer


Messages : 539
Date d'inscription : 2009-07-14
Age : 34

PostSubject: Re: Here are some fixes for some issues in the code.   Tue Mar 15, 2011 3:01 pm

Ok i ve fixed some stuff on the svn and will release a maintenance version

Here are my comments :

*BUGS*
1. Fixed
2. Fixed - Yeah that's more optimized
3. Fixed
4. Fixed
5. Fixed - I have made the destructor virtual but have left the createBuffer private. Derived classes can implement it but must not call their parent's createBuffer
6. Fixed - I have kept the polymorphism but changed the signature so that it occurs. A cast is performed when needed inside the method
7. Fixed - At least for the ones i found

*WARNINGS*
1. Unchanged - I think this is allowed by the norm
2. Unchanged - Unused parameters are mainly in derived implementations of virtual methods, which is normal
3. Fixed - Explicit cast
4. Unchanged - while(true) looks more explicit than for(;; ) and is totally legal
5. Unchanged - That s a choice. I dont like prefixing variables so I m using shadowing. The scope is enough to know which variable is used
6. Unchanged - Even though this could lead to bugs, only independant members are initialized in initialization list. Too painful to be changed
7. Fixed

Thanks again for your feedbacks
Back to top Go down
View user profile http://spark.developpez.com
Sponsored content




PostSubject: Re: Here are some fixes for some issues in the code.   Today at 9:28 am

Back to top Go down
 
Here are some fixes for some issues in the code.
View previous topic View next topic Back to top 
Page 1 of 1
 Similar topics
-
» Security Issues in Selenium IDE
» Has Anyone Gotten Chrome Driver to Work with Windows 7?
» How to solve problem for dynamic ID changes using selenium
» Got My Copy of Nintendo Power's Final Issue!
» horrible inner ear issues..anyone else delt with this

Permissions in this forum:You cannot reply to topics in this forum
 :: English Forum :: Evolution (en)-
Jump to: