Re: [simspark-devel] APL Completion

[Hedayat V. <hed...@ai...>]

Hi again,

On ۱۰/۰۱/۲۹  07:44, ah...@un... wrote:
> Hi,
>
> First of all, thanks for the suggestions.
>    
:) You are welcome.

> 1) I admit that the paper is currently misleading about these "invisible"
> implementation objects. The only problem that was directly caused by this
> was that storing variables inside the implementation objects created the
> possibility of unwillingly using a different object where that variable is
> still zero or a null pointer. This has been completely ruled out by not
> storing anything inside the implementations.
>
> The problem with the Joint classes is caused by a pretty tricky design,
> which defines two pure virtual methods, SetParameter and GetParameter. It
> implements a load (around 20) other methods that call SetParam and
> GetParam with ODE-specific constants. Subclasses of Joint then inherit the
> implemented methods and overwrite SetParam and GetParam so it will work.
>
> However, since the methods that call GetParam and SetParam use
> engine-specific constants, I have to implement them in ODEJoint. But I
> don't know in ODEJoint whether I will have to call ODEBallJoint::SetParam
> or ODEHingeJoint::SetParam etc. Try looking at joint.cpp, hingejoint.cpp
> and odejoint.cpp to see what is going on, I admit that this is VERY hard
> to explain.
>    
OK, I'll checkout your branch and take a look at the code to better 
understand what you are talking about.

> I thought it was impossible to solve this in any other way than the one I
> use currently, but now that I think about it, deriving ODEBallJoint from
> Joint instead of ODEJoint (which I think is what you suggested) might
> actually solve the problem. I'll have to test if this has any side
> effects, but I'll definitely try it out.
>    
Nice. And I'll try to know more about the code till then ;)

> 2) That's some great input. Maybe I can get rid of the
> StaticPhysicsMethods class completely by declaring the pointers to the
> implementation objects as static, which I should do anyway now that I
> think about it. Then, I can use these within the static methods and
> implement these with the bridge pattern as usual. I'll try this out.
>
> 3) So, if I understand correctly, you want me to add the ImpFactory into
> the scenegraph and then retrieve the parent ImpFactory node of a physics
> object instead of using a GetInstance method?
>    
Yes, something like this (as the factory is a single object, it should 
be probably in an address like "/server/physicsobjectfactory" and 
referenced by this rather than using relative addressing like getting 
the factory of the parent node).

> Joschka has already said that with the Factory I made this week, it should
> be possible to write a ruby script that allows choosing the engine at
> runtime. I have too little knowledge of both zeitgeist and ruby to judge
> the likeliness of that possibility, though. However, Joschka and I planned
> to look at this anyway, so I'll keep this at the back of my head.
>    
:) So you are going with that anyway. That's nice.

> And yeah, I isolated the engine-specific code pretty well at this point.
>    
Great!

Thanks a lot,
Hedayat
>
> thanks again,
>
> Andreas
>
>    
>> Thanks a lot for all your efforts in developing the APL.
>> I have a few comments regarding the paper and also your recent commits.
>> 1. (This might be out dated with regard to the recent commits) About the
>> Implementation classes: In the paper, you've said that some classes have
>> 2 instances of their parent implementation class (e.g. SphereColliderImp
>> has two instances of ColliderImp class): one of them through direct
>> inheritance and another one inherited from the Collider class. My
>> suggestion is to eliminate the inherited one and use the inherited
>> Collider instance instead (as it functions as a gateway for the internal
>> Implementation class) or get the Implementation object of the Collider
>> class if the implementation object itself is needed and the
>> functionality is not provided through the Collider class (I might have
>> missed some points as I'm not aware of the code very much).
>> It seems that you've changed the design and now there is only one object
>> of each Imp. class globally. If it is possible (so the Imp. classes
>> probably doesn't have any variables), so this is a good solution too. :P
>> And their case will be similar to the case of static functions
>> (explained below).
>>
>> 2. The Static Functions: if I've understood correctly, there are a set
>> of static methods with engine specific implementation. Instead of having
>> a class with different implementations based on the selected physics
>> engine, my proposal is to either use a single object of an engine
>> specific implementation and call its member functions (the object
>> reference can be obtained from either from the Zeitgeist framework or
>> having a static GetSingleton() function in the StaticMethods class), or
>> by using the bridge pattern for the StaticMethods class too: this class
>> could have a static private refrence to an implmentation class which is
>> used by the static functions.
>>
>> 3. In your recent commit, you've introduced a factory class which
>> currently contains the instances of Imp. classes. I've two comments
>> about it: first, the same pattern have been used throughout the code but
>> with a slightly different style: references are stored in the zeitgeist
>> hierarchy and accessed using its methods. You might use the same style
>> for more consistency (or might not!).
>> Second, the instance of the factory class itself is usually obtained
>> using Zeitgeist framework too, instead of calling a static method like
>> TheFactory::GetInstance(). Beside from consistency point of view, this
>> style provides the flexibility to create the desired object in .rb
>> scripts (the actual engine is selected there). (Notice that I've NOT
>> checked to see where you create the actual factory instance, so your
>> solution might be as flexible as this one!).
>> Also you could use this factory to create any engine specific objects
>> and not only storing single instances (but apparently you do not have
>> any such engine specific classes and all of them are single instance
>> classes?!).
>>
>> Finally, I feel that you are reaching a point where you can completely
>> detach the ODE specific code into a separate plugin rather than being in
>> the Oxygen library itself. This would be nice! :)
>>
>> Thanks a lot,
>> Hedayat
>>
>>
>>
>>      
>>> thanks,
>>>
>>> Andreas
>