Samsung / GearVRf

The GearVR framework(GearVRf) is an Open Source VR rendering library for application development on VR-supported Android devices.
http://www.gearvrf.org
Apache License 2.0
407 stars 217 forks source link

Multiple constraints to same scene object #1893

Closed ragner closed 6 years ago

ragner commented 6 years ago

Change Component API to support multiple component of same type(not exclusive): 1) GVRComponent::isExclusive() - To check if the component is of type exclusive. It is not possible add more than one component of same type when it is exclusive. Whether the the component is not exclusive you can add as much as you need. 2) GVRComponent::next() - Returns the next component os same type attached to the same scene object. Will be always null for exclusive component.

Makes all constraints not exclusive type: 3) GVRConstraint::isExclusive() - Returns false.

Test to this pull request: https://github.com/gearvrf/GearVRf-Tests/pull/276

After this PR the attach/detach of components will work almost like Unity3D api.

GearVRf-DCO-1.0-Signed-off-by: Ragner Magalhaes <ragner.n@samsung.com>

NolaDonato commented 6 years ago

I do not see any reason for the concept of "exclusive". Any group of components is "exclusive", there is no need to have a group of components of different types. In PR #1874 addChildComponent checks to see if the type of the child is the same as the type of the group. That should be all you need.

ragner commented 6 years ago

Hi @NolaDonato, I am just proposing for the framework the same behavior than Unit3D for components attachment.

I am adding the concept of "exclusive" just to identify components like GVRTransform that allows just a single instance attached to same scene object.

Colliders, Constraints, spatial AudioSource, etc are "not exclusive".

NolaDonato commented 6 years ago

So you want a flag to identify groups vs single components? This is why I proposed to make GVRComponentGroup an interface. Then it would be easy to identify groups. I think this change adds little to no overhead in Java and uses the language facilities rather than a separate flag to make the distinction.

NolaDonato commented 6 years ago

OK I added an interface GVRComponent.IComponentGroup to PR #1874. It contains addChildComponent and removeChildComponent. If your component type implements this interface, it is "non-exclusive". You can check this with (mycomponent instanceof GVRComponent.IComponentGroup) in Java. GVRColliderGroup implements this interface. Your constraint groups can as well. This is the initial solution I proposed a few weeks ago. It is cleaner than adding special flags or having C++ multiple inheritance. It lets the C++ classes implement adding the components any way they want.

ragner commented 6 years ago

I was just trying to avoid to implement a group class for each "non-exclusive" component type that looks common.

SceneObject obj;
obj.attachComponent(colliderA)
obj.attachComponent(colliderB)
obj.attachComponent(colliderN)
...
c = obj.getComponent(collider type);
// go through all colliders attached to obj
while(c != null) {
  c = c.next();
}

It looks that I cannot figure out an acceptable alternative to "group", So we are going to implement a group for constraints and groups for each other "non-exclusive" component type.

Thank you for your review.

liaxim commented 6 years ago

@ragner Are you still planning to use a group of constraints and https://github.com/Samsung/GearVRf/pull/1904? Is there an agreement or should we discuss further?

ragner commented 6 years ago

Hi @liaxim, it doesn't look good to me the ideia of different Group class for every "non-exclusive" component like ConstraintGroup, ColliderGroup, SpatialAudioGroup, etc. It looks hard to mantain a lot of groups.

In my opinion should be possible attache multiple "non-exclusive" components to a scene object or we should have a "Generic" ComponentGroup that works for all "non-exclusive" components and we should handle the special cases with specialized group types.

But it is my opinion and probably I may be wrong.

liaxim commented 6 years ago

@ragner It is a reasonable request. Would something like https://github.com/Samsung/GearVRf/pull/1911 help? I don't want to go into the collider group now so there will be some overlap.

ragner commented 6 years ago

Hi @liaxim, #1911 looks nice to me. Thanks

ragner commented 6 years ago

After #1911 this PR looks not necessary anymore. I will try other on top of #1911 that looks good to me, thanks