Add error checks for bad configuration in PathFollow2D/3D set_progress_ratio

[gturri]

When a PathFollow2D is badly configured it's possible to have code that a call to get_progress_ratio just after a set_progress_ratio, does not return the value just set, which may be confusing for developer (ie that

  myPathFollow2D.set_progress_ratio(0.5)
  myPathFollow2D.get_progress_ratio()

does not return 0.5)

This commit makes sure the developer has a useful error messages in such case, to make it easier to troubleshoot it.

[AThousandShips]

This introduces errors where none were before, this is not necessarily appropriate, also makes the code significantly different from the 3D case

[gturri]

Thanks for this feedback.

About

This introduces errors where none were before

Yes, correct. And it turns out that's the point.

Some context (that I could have shared earlier, sorry for that): I had code like this

var path = Path2D.new()
var curve = Curve2D.new()
curve.add_point(Vector2.ZERO)
curve.add_point(Vector2(1, 1))
path.curve = curve
var pathFollow2D = PathFollow2D.new()
path.add_child(pathFollow2D)

pathFollow2D.progress_ratio = 0.42
print("progress_ratio: " + str(pathFollow2D.progress_ratio))

and it took me dozens of minutes to understand why the value printed was 0 instead of the 0.42 I set just a line above.
I eventually understood it because I fortunately did some C++ in a previous life and could hence read the godot code. So I could understand that my issue was that path was null, and understand that the way to have it non-null is to add the parent Path2D to the tree scene.
I guess that those error messages not only could have saved me a bit of time, I also guess that some people less fluent in C++ could appreciate some error message to have clues about what they are doing wrong.

About

makes the code significantly different from the 3D case

thanks for drawing my attention on it. It indeed would make sense to have in the 3D case as well.

--
So I guess my question now is: does it make sense that I apply the suggested changes (namely:

• fix formatting
• use ERR_FAIL_NULL_MSG
• make it explicit in the first error message that the parent should be a Path2D
• apply the same change in the 3D case
  )

or do you feel it's a change that you would not want merged anyway?

[AThousandShips]

I think this might be better to document instead, but I'm fine with either way, but:

• This should be in both 2D and 3D or neither IMO
• It should also be documented in either case

[gturri]

Thanks for that answer.

Just to make sure I understand correctly: when you say that it should be documented, do you mean that I should do a PR on the documentation repo in order to make it clear on the doc of the set_progress_ratio field that it cannot be changed in the cases identified (ie: are we talking of a PR on this: https://github.com/godotengine/godot-docs/blob/4b755c0dac449ec9cd899dbf39d7619fb0edba2e/classes/class_pathfollow2d.rst#L13 ), or do you have some other documentation in mind?

[AThousandShips]

No it should be here, documentation is generated from doc/classes/*.xml files, that should be added in this PR as well, or as an alternative PR just documenting this limitation

[gturri]

Thanks for this pointer.

I should be able to take the time to do this change today or tomorrow 👍

[gturri]

I just updated that pull request accordingly, with both the changes in the code (fixes for PathFollow2D + applied the changes for PathFollow3D) and updated the doc.

Don't hesitate to let me know if there are other things you'd rather I change.

[KoBeWi]

I wonder if get_progress_ratio() should also have similar error. Technically this will fail silently too:

myPathFollow2D.progress = 100
print(myPathFollow2D.progress_ratio) # 0

[gturri]

I wonder if get_progress_ratio() should also have similar error

yes, correct: it has the same behavior. That's why the doc updated by this commit also mentions the getter.

I did not add the ERR_FAIL_NULL_MSG & co in the getter because I though that if someone manipulates a PathFollow2D when e.g. the Path2D is not part of the scene tree, then he or she will already have an error message upon calling the setter, which already makes it easy to understand what is going wrong.
(also, in the 1st comment on this PR someone expresses concerns about introducing new errors, so I figured out it would be better to not add more of those).

That being said, if you believe it would be better to emit those error messages in the getter too, I can amend this PR.

[gturri]

Hi there,

I understand that you are all pretty busy with other stuff (like summer holidays, or shipping godot 4.3 (thanks btw), or anything else), so it's cool that this PR is on hold.
On the other hand, if the current status is that something is expected from me in order to make this PR progress, please don't hesitate to let me know :)

[akien-mga]

Makes sense to me.

I force pushed an amend of the commit to make it match the PR title, which is more informative.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉