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.

Sign up for GitHub

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

Jump to bottom

Fix hit animation not synchronizing when editing hit objects #24508

Merged
merged 11 commits into from
Aug 22, 2023
Merged

Fix hit animation not synchronizing when editing hit objects #24508

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d2f5e69
Fix DrawableHitObject state not synchronizing with hitobject edits
OliBomby Aug 11, 2023
5d1ccc2
Ensure invariant of monotone time
OliBomby Aug 11, 2023
90f2aca
Fix typo
OliBomby Aug 11, 2023
b9d0a8a
Fix TestSceneFruitRandomness
OliBomby Aug 13, 2023
82de738
Revert "Fix TestSceneFruitRandomness"
OliBomby Aug 21, 2023
c7b1c75
Revert "Fix typo"
OliBomby Aug 21, 2023
5bc11ed
Revert "Ensure invariant of monotone time"
OliBomby Aug 21, 2023
c82e997
Revert "Revert "Fix TestSceneFruitRandomness""
OliBomby Aug 21, 2023
e283aa2
Update inline comments
OliBomby Aug 21, 2023
5be5335
Reword comment to be better
bdach Aug 22, 2023
8cd9f08
Merge branch 'master' into judge-fix
bdach Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions osu.Game.Rulesets.Catch.Tests/TestSceneFruitRandomness.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ public TestSceneFruitRandomness()
AddSliderStep("start time", 500, 600, 0, x =>
{
drawableFruit.HitObject.StartTime = drawableBanana.HitObject.StartTime = x;
drawableFruit.RefreshStateTransforms();
drawableBanana.RefreshStateTransforms();
bdach marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand All @@ -44,6 +46,8 @@ public void TestFruitRandomness()
AddStep("Initialize start time", () =>
{
drawableFruit.HitObject.StartTime = drawableBanana.HitObject.StartTime = initial_start_time;
drawableFruit.RefreshStateTransforms();
drawableBanana.RefreshStateTransforms();

fruitRotation = drawableFruit.DisplayRotation;
bananaRotation = drawableBanana.DisplayRotation;
Expand All @@ -54,6 +58,8 @@ public void TestFruitRandomness()
AddStep("change start time", () =>
{
drawableFruit.HitObject.StartTime = drawableBanana.HitObject.StartTime = another_start_time;
drawableFruit.RefreshStateTransforms();
drawableBanana.RefreshStateTransforms();
});

AddAssert("fruit rotation is changed", () => drawableFruit.DisplayRotation != fruitRotation);
Expand All @@ -64,6 +70,8 @@ public void TestFruitRandomness()
AddStep("reset start time", () =>
{
drawableFruit.HitObject.StartTime = drawableBanana.HitObject.StartTime = initial_start_time;
drawableFruit.RefreshStateTransforms();
drawableBanana.RefreshStateTransforms();
});

AddAssert("rotation and size restored", () =>
Expand Down
17 changes: 11 additions & 6 deletions osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,6 @@ protected sealed override void OnApply(HitObjectLifetimeEntry entry)
}

StartTimeBindable.BindTo(HitObject.StartTimeBindable);
StartTimeBindable.BindValueChanged(onStartTimeChanged);

if (HitObject is IHasComboInformation combo)
{
Expand Down Expand Up @@ -311,9 +310,6 @@ protected sealed override void OnFree(HitObjectLifetimeEntry entry)

samplesBindable.UnbindFrom(HitObject.SamplesBindable);

// Changes in start time trigger state updates. When a new hitobject is applied, OnApply() automatically performs a state update anyway.
StartTimeBindable.ValueChanged -= onStartTimeChanged;

// When a new hitobject is applied, the samples will be cleared before re-populating.
// In order to stop this needless update, the event is unbound and re-bound as late as possible in Apply().
samplesBindable.CollectionChanged -= onSamplesChanged;
Expand All @@ -333,6 +329,8 @@ protected sealed override void OnFree(HitObjectLifetimeEntry entry)
Entry.NestedEntries.RemoveAll(nestedEntry => nestedEntry is SyntheticHitObjectEntry);
ClearNestedHitObjects();

// Changes in state trigger defaults applied trigger state updates.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sounds circular. What are the "changes in state" at the start there? Hitobject property changes? Not literal changes to DrawableHitObject.State?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "changes in state" refers to state of the HitObject

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated comment to be clearer in 5be5335.

// When a new hitobject is applied, OnApply() automatically performs a state update.
HitObject.DefaultsApplied -= onDefaultsApplied;

entry.RevertResult -= onRevertResult;
Expand Down Expand Up @@ -375,8 +373,6 @@ protected virtual void LoadSamples()

private void onSamplesChanged(object sender, NotifyCollectionChangedEventArgs e) => LoadSamples();

private void onStartTimeChanged(ValueChangedEvent<double> startTime) => updateState(State.Value, true);

private void onNewResult(DrawableHitObject drawableHitObject, JudgementResult result) => OnNewResult?.Invoke(drawableHitObject, result);

private void onRevertResult()
Expand All @@ -394,6 +390,15 @@ private void onDefaultsApplied(HitObject hitObject)
Debug.Assert(Entry != null);
Apply(Entry);

// Applied defaults indicate a change in hit object state.
// We need to update the judgement result time to the new end time
// and update state to ensure the hit object fades out at the correct time.
if (Result is not null)
bdach marked this conversation as resolved.
Show resolved Hide resolved
{
Result.TimeOffset = 0;
updateState(State.Value, true);
}

DefaultsApplied?.Invoke(this);
}

Expand Down
Loading