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

New error poped up in 3.5 beta 5 which isn't present in beta 4 with navigation in 3d. #60735

Closed
viksl opened this issue May 3, 2022 · 8 comments

Comments

@viksl
Copy link
Contributor

viksl commented May 3, 2022

Godot version

3.5b5 mono

System information

Windows 10, Nvidia gtx 1660Ti, latest drivers, GLES3

Issue description

I have a test scene with the new navigation in 3d to see how it works.
I moved from beta 4 to beta 5 today and now I get this error which doesn't show in beta 4 (I went back to b4 to double check it).

Everything is in the video here: https://youtu.be/LWD3xOZgDWk

It occurs more when I lower the Target Desired Distance of the NavigationAgent I use.

I don't know if this is a problem with my code or a bug since it appears only in the newer build (b5).

Godot 3 5b5

Steps to reproduce

  1. Open the project (Beta5 mono version!).
  2. Run the project.
  3. Let the agents move and then check the debugger, leave them around the end point for a bit with lower desired distance to get the error more times.

Open the project in Beta4 and repeat the steps above to see no error.

Minimal reproduction project

NavTest.zip

@Calinou
Copy link
Member

Calinou commented May 3, 2022

cc @lawnjelly

@lawnjelly
Copy link
Member

lawnjelly commented May 4, 2022

Note: For your MRP we prefer projects that use only gdscript, unless c# is part of the problem (I can't run it here).

I'm presuming you have physics interpolation switched on?

This is a new set of optional warnings introduced in beta 5 in #60531. They can be disabled in project_settings/debug/settings/physics_interpolation/enable_warnings, and they only appear in debug builds.

However they are indicating that a VisualInstance (that is interpolated) is having set_transform() called during _process() (a lot). This will give incorrect results with physics interpolation, and is indicating to you (or the contributors, it may be a bug in core) that you / we should fix this. This applies to nodes that are moved both directly, and indirectly (via moving a parent node in the scene tree, or via an "automatic" movement method).

See the preliminary docs : https://github.com/lawnjelly/Misc/blob/master/FTIDocs/FTI.md
(I'm currently finishing these to go in the official docs).

Objects that are interpolated generally should only have their transforms set during _physics_process() and not _process(). Turning physics interpolation on is not always a simple case of switching it on, you often have to make changes to the project in order for it to work correctly.

User game logic code should be moved to _physics_process(). In addition, if you are using nodes / features that automatically move interpolated nodes, these must currently also be set to work in physics mode rather than idle.

Some that have come up as culprits during testing so far are:

  • Tween node
  • Animation player

It is very possible the navigation node also assumes it can move objects during _process(). This is not the case with physics interpolation, and either it needs to be toggled to operate on the physics tick, or we need to add a mode to do this before 3.5 stable.

Another option I have been discussing with @RPicster is whether we can simply have these nodes automatically change to physics process when they are being used with an interpolated node. He has also suggested we modify the warning to print the nodepath as well as the node name, which makes a lot of sense (as a lot of cases you might have multiple nodes called "MeshInstance").

This is probably something we will aim to perfect before 3.5 stable, the fact that there was no warning previously did not mean there was not a problem in earlier builds (you just may not have seen it).

One further point, the warning is currently rather "dumb". It just detects a threshold number of calls outside of the physics tick. If you are for example, teleporting objects in _process(), or at startup of a level, this is not necessarily a problem, the problem comes with continuously using set_transform() on a per frame basis. This is why the warning says "possibly benign".

@viksl
Copy link
Contributor Author

viksl commented May 4, 2022

@lawnjelly
Well, I'll try to keep this brief :).

I just checked and it's true that unchecking the physics interpolation ledas to no errors in the debugger.

When it comes to gdscrip,t I'm in C# so if needed I guess I could change it to gdscript (if really necessary) but since this is a problem that poped up on a mono build and I use C# I added the test scene so it's as close to my own setting as possible.

In case you decide to check the test demo it's only navigatoin with some navigation agents and meshes but the agents themselves are RigidBodies so I don't use any transforms directly anywhere at all, I also don't use Tweens or Animation player.

Maybe my script is flawed in this but from other use cases I understood this is how you use it:

// Ready override
_navigationAgent.SetNavigation(navigation);
_navigationAgent.SetTargetLocation(_target.GlobalTransform.origin);

// PhysicsProcess override
var velocity = GlobalTransform.origin.DirectionTo(_target.GlobalTransform.origin) * _speed;

if (!_navigationAgent.IsTargetReached())
{
    _navigationAgent.SetVelocity(velocity);
}

private void OnVelocityComputed(Vector3 safeVelocity)
{
    LinearVelocity = safeVelocity;
}

All the examples so far I've seen used it like this, the RigidBody is set to Character mode with Physics Material override to 0 friction.

Maybe setting the LinearVelocity in the OnVelocityComputed method (connected to the appropriate signal in NavigationAgent) should be made differently, because since I don't really have anything else I don't see what transforms could be changing then?

I tried a deferred call instead like this:

    private void SetVelocityDeferred(Vector3 safeVelocity)
    {
        LinearVelocity = safeVelocity;        
    }

    private void OnVelocityComputed(Vector3 safeVelocity)
    {
        CallDeferred(nameof(SetVelocityDeferred), safeVelocity);
    }

But the result is the same I still get the errors, I do not use Process at all so that's also out of the question at least from my side.
(I have a process on the main scene node to dispaly fps as a text in a label to be precise but that has no influence on the matter even if I remove it).

I've gone through all the nodes and their Inspector but I can't find where I'd be able to switch them to physics from idle, at least not on RigidBody which I'd expect does this automatically, MeshInstance, NavigationAgent or Camera. I don't use other nodes (well apart from my camera setting and some UI container to Stick the fps label to the screen but I've removed all these nodes completely from the scene and the error still popups.

I'm sorry I'm probably just dumb but I don't know where else to look to fix this? I mean I don't change Transforms anywhere at all in my scripts and the demo is really just made to test the basic navigation it's not even from my game with some extra code or so.

Having the name in the error would certainly help to track where it problem comes from since the rest of the error points to cpp (c++) file which doesn't provide much info on it's own for sure ;).

Would you please mind telling me what I've overlooked, I'm really probably just stupid here?
Thank you very much.

EDIT: I also don't teleport anything on my own at the start, I have the scene already set up in the editor from the beginning. Also the errors start appearing after several seconds of the navigation in the middle of nothing really happening so it's working for a while and then suddenly Error, then again then again. That's why I'm a bit confused since I don't do anything at that moment different, I don't even have any controls, the target point is set from the beginning as is in the code above illustrated.

But since it's a new system without examples of usage I think I might be using it wrong in some way but this is the way I found to use it so far and seems to work apart from the error popups.

EDIT2: I just noticed your link to the interpolation I'll double check it right now.

EDIT3: Checked the steps, I have no Process, I called the interpolation reset on everything even if it doesn't move. No changes.
I must be doing something else wrong.

@lawnjelly
Copy link
Member

Without a gdscript MRP and being able to debug it, there are a lot of things that could be causing it, very likely the core navigation might need tweaking (rather than a problem on your side). This is why we have betas! 🙂

If anyone can make a MRP with a simple navigation example in gdscript, with physics interpolation switched on that shows the warning I can investigate and fix this.

@viksl
Copy link
Contributor Author

viksl commented May 4, 2022

@lawnjelly I'll make the gdscript example and post it today here in the comments I guess? Or at least let you know if I get the same error or not. xD

@viksl
Copy link
Contributor Author

viksl commented May 4, 2022

@lawnjelly
I think I set the project the same here in gdscript, I hope I didn't miss something, I don't use GDScript so let me know if you feel there's something completely off please :).
The error still pops up
NavTestGDS.zip
.

@lawnjelly
Copy link
Member

lawnjelly commented May 4, 2022

Thanks the project is working great. 👍

I'm just debugging, may have to finish tomorrow morning. I suspect we may need to flush the transform notifications after running the physics server when using interpolation, it will be something like that. I don't think there is a problem in your code.

EDIT: Yes flushing the transforms seems to fix it. I'll need to research this a bit more why it only breaks in this particular case in the morning (i.e. why was it ever working 😁 ).

@akien-mga
Copy link
Member

Fixed by #60763.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants