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

PathFollow3D progress and progress_ratio properties not working after upgrading from 4.2.2 to 4.3 #95612

Closed
AaronJMorand opened this issue Aug 16, 2024 · 18 comments · Fixed by #96140

Comments

@AaronJMorand
Copy link

Tested versions

  • Reproducible 4.3.stable
  • Works in 4.2.2.stable

System information

Godot v4.2.2.stable - Debian GNU/Linux 12 (bookworm) 12 - X11 - GLES3 (Compatibility) - NVIDIA GeForce GTX 960 (nvidia) - AMD FX(tm)-6300 Six-Core Processor (6 Threads)

Issue description

PathFollow3D progress and progress_ratio properties are not working after upgrading from 4.2.2 to 4.3. Running my game on 4.2.2 again afterwards works as expected. In the inspector, I can manually adjust the properties and see the position move in the 3D view.
The full project is shared on GitHub : https://github.com/Pixel-Gladiator/LaserBall3D
Load it in 4.2.2 and the world resources populate as expected.
Load it in 4.3 and the world resources populate in a single location.
In both cases, world resources spawning off of a timer are added as expected.
I've shared videos of what it used to look like and what it looks like now on YouTube
https://www.youtube.com/channel/UCIIdIVhHBYRUmjfhPLQyY5Q

Steps to reproduce

Load it in 4.2.2 and the world resources populate as expected.
Load it in 4.3 and the world resources populate in a single location.
In both cases, world resources spawning off of a timer are created as expected.

Minimal reproduction project (MRP)

N/A

@AThousandShips
Copy link
Member

Please upload a minimal reproduction project, a full game project is not a minimal project, a minimal project makes things easier to test and confirm without having to dig through a lot of unnecessary information:

  • A small Godot project which reproduces the issue, with no unnecessary files included. Be sure to not include the .godot folder in the archive (but keep project.godot).
  • Having an MRP is very important for contributors to be able to reproduce the bug in the same way that you are experiencing it. When testing a potential fix for the issue, contributors will use the MRP to validate that the fix is working as intended.
  • Drag and drop a ZIP archive to upload it (max 10 MB). Do not select another field until the project is done uploading.
  • Note for C# users: If your issue is not C#-specific, please upload a minimal reproduction project written in GDScript. This will make it easier for contributors to reproduce the issue locally as not everyone has a .NET setup available.

@AaronJMorand
Copy link
Author

Here is a minimal recreation.
PathFollow3D_issue.zip

@kleonc
Copy link
Member

kleonc commented Aug 16, 2024

That's a regression from #80233 (happens since 4.3.dev4), which made changing all relevant PathFollow3D properties result in deferred transform update, instead of updating the transform immediately (PathFollow3D::update_transform is always called with the default p_immediate = false).
cc @xiongyaohua

godot/scene/3d/path_3d.cpp

Lines 463 to 467 in a759867

void PathFollow3D::set_progress_ratio(real_t p_ratio) {
if (path && path->get_curve().is_valid() && path->get_curve()->get_baked_length()) {
set_progress(p_ratio * path->get_curve()->get_baked_length());
}
}

godot/scene/3d/path_3d.cpp

Lines 415 to 435 in a759867

void PathFollow3D::set_progress(real_t p_progress) {
ERR_FAIL_COND(!isfinite(p_progress));
progress = p_progress;
if (path) {
if (path->get_curve().is_valid()) {
real_t path_length = path->get_curve()->get_baked_length();
if (loop && path_length) {
progress = Math::fposmod(progress, path_length);
if (!Math::is_zero_approx(p_progress) && Math::is_zero_approx(progress)) {
progress = path_length;
}
} else {
progress = CLAMP(progress, 0, path_length);
}
}
update_transform();
}
}

void update_transform(bool p_immediate = false);

godot/scene/3d/path_3d.cpp

Lines 220 to 228 in a759867

void PathFollow3D::update_transform(bool p_immediate) {
transform_dirty = true;
if (p_immediate) {
_update_transform();
} else {
callable_mp(this, &PathFollow3D::_update_transform).call_deferred();
}
}

@akien-mga akien-mga added this to the 4.4 milestone Aug 16, 2024
@AlbeyAmakiir
Copy link

Is there a workaround for now?

@bjr29
Copy link

bjr29 commented Aug 19, 2024

Is there a workaround for now?

I also really need to know if there's a workaround for this. I need this feature and I can't use the 4.2 version as my engine due to needing the features of 4.3.

@AlbeyAmakiir
Copy link

Actually, depending what you need, this workaround might not be too hard. The progress updates just fine, so feeding that into, say, curve.sample_baked_with_rotation(pathfollow.progress) will get you the whole Transform3D to play with.

@Mickeon
Copy link
Contributor

Mickeon commented Aug 19, 2024

What to do, then? Should that PR be reverted? Should the transform be updated immediately (likely yes)?

@kleonc
Copy link
Member

kleonc commented Aug 19, 2024

What to do, then? Should that PR be reverted? Should the transform be updated immediately (likely yes)?

Reverted no, it fixed some other issues. From #80233 (comment):

This PR fix the following minor issues on PathFollow3D.

  • transform is not updated after setting new flags such as use_model_front
  • transform is not updated when the parent Path3D changes
  • correct_posture() behaves differently when assuming model front
  • _update_transform() was in immediate mode, could leads to chained calls on scene instantiation.

Seems like the updates were made to be deferred because of the last bullet point. Aka the intent was to avoid many updates on scene instantiation, but deferring was made to happen always instead of only on instantiation... 🙃

So yes, always updating transform immediately should do as a simple hotfix. Somehow avoiding multiple updates on scene instantiation (according to the original intent) would be a nice bonus of course.

@Mickeon
Copy link
Contributor

Mickeon commented Aug 19, 2024

I trust your judgement. I believe this needs to be fixed ASAP, a lot of people I know personally rely on this logic.

@xiongyaohua
Copy link
Contributor

yeah, make p_immediate=true as the default for update_transform() should fix the problem.

@seva-dobr
Copy link

as a side note: force_update_transform() doesn't seem to work either. i don't really know if it's related or not.

@wirelessfuture
Copy link

I was going crazy today wondering why progress_ratio wasn't working and now I see it 🥇. Thanks for stopping my insanity from progressing.

@wirelessfuture
Copy link

Is there a workaround for now?

Subtracting delta from progress_ratio seems to work for now, but there is a noticeable delay in movement, at least for me.

@wirelessfuture
Copy link

wirelessfuture commented Sep 11, 2024 via email

@brickyt
Copy link

brickyt commented Sep 12, 2024

How can I get this fix locally? Sorry for the noob question, I've never had to track a needed fix for Godot before. Do I redownload 4.3?

@AaronJMorand
Copy link
Author

AaronJMorand commented Sep 12, 2024

Unless you're building Godot from source, it will be available from here https://godotengine.org/download/preview/
I don't think the fix is in Godot 4.4-dev2 though, based on when the commits for this fix and when dev2 released. I'd imagine Godot 4.4-dev3.

@brickyt
Copy link

brickyt commented Sep 12, 2024

Thanks @AaronJMorand! How do I track which release it gets into? Sorry if this is an obvious question.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 12, 2024

By the milestone (it says 4.4 in the top right) and also on the PR with the cherry pick labels (in this case 4.3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Very Bad
Development

Successfully merging a pull request may close this issue.