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

Feat/snap zone prep #378

Merged
merged 20 commits into from Apr 16, 2019

Conversation

Projects
None yet
2 participants
@thestonefox
Copy link
Member

thestonefox commented Apr 14, 2019

No description provided.

@thestonefox thestonefox requested a review from bddckr Apr 14, 2019

Show resolved Hide resolved ...ation/Operation/Extraction/TransformPropertyApplierEventDataExtractor.cs
Show resolved Hide resolved Runtime/Tracking/Modification/TransformPropertyApplier.cs Outdated
@@ -152,8 +152,7 @@ public class UnityEvent : UnityEvent<EventData>
/// <param name="source">The data to build the new source from.</param>
public virtual void SetSource(GameObject source)
{
sourceTransformData.Clear();
sourceTransformData.Transform = source != null ? source.transform : null;
Source = source != null ? new TransformData(source) : null;

This comment has been minimized.

Copy link
@bddckr

bddckr Apr 15, 2019

Member

A suggestion that could work and means there are still no GC'ed allocations:

public virtual void SetSource(GameObject source)
{
    sourceTransformData.Clear();
    sourceTransformData.Transform = source != null ? source.transform : null;
    Source = sourceTransformData;
}

Then down the file in the change handler bail out early, to ensure we don't override the things SetSource just did:

[CalledAfterChangeOf(nameof(Source))]
protected virtual void OnAfterSourceChange()
{
    if (Source == sourceTransformData)
    {
        return;
    }

    sourceTransformData.Clear();

    if (Source == null)
    {
        return;
    }

    sourceTransformData.Transform = Source.Transform;
    sourceTransformData.UseLocalValues = Source.UseLocalValues;
    sourceTransformData.PositionOverride = Source.PositionOverride;
    sourceTransformData.RotationOverride = Source.RotationOverride;
    sourceTransformData.ScaleOverride = Source.ScaleOverride;
}

This comment has been minimized.

Copy link
@thestonefox

thestonefox Apr 16, 2019

Author Member

As mentioned in slack. I just wonder if trying to avoid GC here is going against the normal operation of setting a new source.

If you do this to set the Source via code:

TransformData myData = new TransformData();
applier.Source = myData;

Then you're effectively doing the same as the SetSource(GameObejct) method.

thestonefox added some commits Apr 13, 2019

refactor(Collection): reorganize collections into sub namespaces
The number of differing collections along with their concrete
implementations was making the Collection directory hard to follow so
each of the collection types that has a base and concrete
implementations has now been moved into a sub directory along with a
sub namespace to clean up the Collection directory.
refactor(Operation): reorganize operations into sub namespaces
The Operation namespace had multiple operation types in the same
directory level and was becoming messy to see what the operation
types were. They have now been moved into sub directories to tidy
up the namespace.
refactor(Transformation): reorganize into sub namespaces
The Transformation namespace had different types of transformation
in the same directory level and was becoming messy to see what the
transformation was. They have now been moved into sub directories to
tidy up the namespace.
refactor(Event): reorganize events into sub namespaces
The Events namespace had multiple types of event emissions in the same
namespace which made it confusing to understand the separation between
types. The events have now been split into sub namespaces to clean up
any confusion.

Any events that are specific to another namespace type have also been
moved to the appropriate namespace and directory.
feat(Operation): move extractor emitters to operation extraction
Previously, there were GameObject extractors known as Event Emitters
but really these are just doing an Extraction Operation on a certain
DataType so really these components should reside within the
Data/Operation/Extraction namespace.
feat(Collection): check if list contents is empty
A new method for the ObservableList has been added that checks to see
if the collection contents is empty and if it is then it emits an
`IsEmpty` event or an `IsPopulated` event if the collection is not
empty.
feat(Collision): emit when active collision is added or removed
The ActiveCollisionsContainer now has events that emit when a new
active collision occurs and when an existing collision ceases. This
extends the functionality that previously happened where an event
was emitted on the first collision and on the last final collision
ceasing.

The event data payloads have also been changed for the events as
there is no point in emitting the entire collision collection for the
first collision as it will only contain one collision, so it may as
well just emit that single collision. The AllStopped event also now
does not emit anything as when all collisions have ceased then no
collision data will be available to emit anyway so it was just emitting
an empty collection.

A fix to the validity check has also been added to ensure it retrieves
the collider containing transform in case of compound colliders.
fix(Collision): ensure collider containing transform is returned
The NotifierTargetExtractor was only ever reporting the actual
GameObject the colliding collider was on instead of the actual
GameObject the collision was being reported on, which could have been
a parent GameObject if there was a collision collecting Rigidbody to
deal with compound colliders.
fix(Modification): create new TransformData when setting by GameObject
The TransformPropertyApplier `SetSource(GameObject source)` method was
not actually performing the correct logic, when setting the `Source`
via a GameObject, it actually should create a brand new TransformData
from that source GameObject and then set the `Source` property, which
internally performs the relevant caching.

Without this, the cached object would be set up correctly but the
public Source property would be incorrect. This method is a creation
method and is rarely called and therefore any overhead is acceptable.
feat(Modification): expose CancelTransition method as public
The CancelTransition method on the TransformPropertyApplier is now
public as it may be required to cancel the existing transformation
transition externally.
feat(Modification): allow dynamic transition destination
The TransformPropertyApplier destination location for the
transformation can now be specified as a dynamic destination meaning
that any changes to the destination (the `Source` property) that
occur during the transition routine will be applied to the transition
process.

This is useful if the `Source` properties change during the transition
and the `Target` should move to the updated properties instead of the
original specified destination.
feat(Modification): extract event data from TransformPropertyApplier
The TransformPropertyApplier EventData can now be extracted and emitted
via this new component.
feat(Operation): extract related GameObject of TransformData
The TransformDataGameObjectExtractor attempts to extract the related
GameObject of the TransformData stored transform and emits the results.
feat(Follow): store results of extraction in public properties
The ObjectDistanceComparatorEventDataExtractor now has public
properties that store the results of the most recent `Extract()`
execution so the results can be accessed via code as well as
via the event.
refactor(CameraRig): inline variable declaration
It is possible to declare the `out` variable inline rather than
defining the variable before the method is called.
feat(Data): test extraction on inactive components
The ComponentGameObjectExtractorTest now checks the active
state of the GameObejct and component.
feat(Operation): mutate rigidbody settings via GameObject Source
The RigidbodyPropertyMutator exposes rigidbody settings for mutation
as public properties so they can be mutated via UnityEvents but also
the Rigidbody can be specified by providing the containing GameObject
which makes it easier to update Rigidbody data when only the GameObject
is available.
fix(Event): do not mutate data when restrictable proxy is invalid
The RestrictableSingleEventProxyEmitter would still mutate the Payload
data in the ProxyEmitter even if the validity checks failed. This is
not correct behaviour as if something does not pass validity checks
then it should cease to operate and not change any data.

The fix is to ensure the SingleEventProxyEmitter checks the valid
state and if it is not valid then it should reset the payload back
to what it was before the payload was received.

The Payload still needs setting before the validity check is done
because other classes rely on a Payload being available to achieve
the validity check.
feat(Collision): add proxy emitter for CollisionNotifier
The CollisionNotifierEventProxyEmitter allows chaining together events
that emit CollisionNotifier EventData.

It's possible to restrict the proxy event based on either the forward
source of the event data or the collision source.

@thestonefox thestonefox force-pushed the feat/snap-zone-prep branch from 6019a82 to 6bf9a18 Apr 15, 2019

@thestonefox thestonefox requested a review from bddckr Apr 16, 2019

fix(Modification): only use source cache when setting by GameObject
The TransformPropertyApplier was using the source cache internal field
for inline operations but this meant that any changes to the public
Source TransformData property would not be reflected in the internal
code logic as it was never updating the cache unless the Source
property was changed.

This fix removes the source cache from being used inline and always
uses the public Source property. The source cache is now only used
for setting Source with a GameObject via the `SetSource` method.

The use of the source cache here removes the need for creating a
new instance of TransformData from the GameObject and therefore removes
garbage creation.

@thestonefox thestonefox force-pushed the feat/snap-zone-prep branch from 31dc9ce to efd37b2 Apr 16, 2019

@bddckr

bddckr approved these changes Apr 16, 2019

@thestonefox thestonefox merged commit 3c7a037 into master Apr 16, 2019

@thestonefox thestonefox deleted the feat/snap-zone-prep branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.