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

Improve TextureProgressBar.set_radial_initial_angle() by removing loops #98816

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

arkology
Copy link
Contributor

@arkology arkology commented Nov 4, 2024

Replace two while loops with if and fposmodp.
Document radial_initial_angle wrapping.
Infinite numbers will cause error and will not change radial_initial_angle value.
Add testcase for set_radial_initial_angle() argument wrapping and skipping non-finite values.

Should fix #60338.

@arkology arkology requested review from a team as code owners November 4, 2024 14:38
@arkology arkology changed the title Improve set_radial_initial_angle by removing loops Improve TextureProgress.set_radial_initial_angle() by removing loops Nov 4, 2024
@AThousandShips AThousandShips added bug topic:gui cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 4, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 4, 2024
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

See above, does not solve the issue linked (only non-finite inputs)

@arkology
Copy link
Contributor Author

arkology commented Nov 4, 2024

Thanks for review, looks like I opened it too early.

does not solve

Issue was Godot freeze, so technically it solves issue.
I'll add what you suggest.

@arkology arkology marked this pull request as draft November 4, 2024 14:47
@AThousandShips
Copy link
Member

It normally does but it'll cause errors, it also doesn't behave correctly with negative input

@arkology
Copy link
Contributor Author

arkology commented Nov 4, 2024

Yes, for now it doesn't work well with negative input and I will fix it.

@AThousandShips
Copy link
Member

AThousandShips commented Nov 4, 2024

A check with !Math::is_finite and then checking with:

if (p_angle < -360.0 || p_angle > 360.0) {
	p_angle = Math::fmod(p_angle, 360);
}
if (p_angle < 0.0) {
	p_angle += 360.0;
}

Should be sufficient

You could also simply use:

if (p_angle < 0.0 || p_angle > 360.0) {
	p_angle = Math::fposmod(p_angle, 360);
}

And I think some of the checks in that will be optimized out

@arkology arkology force-pushed the to-infinity-and-beyond branch from a3c3e56 to 7cd187c Compare November 4, 2024 15:25
@arkology arkology marked this pull request as ready for review November 4, 2024 15:26
@AThousandShips AThousandShips dismissed their stale review November 4, 2024 15:28

Concerns resolved

@AThousandShips
Copy link
Member

Will test this code when I am able!

@arkology
Copy link
Contributor Author

arkology commented Nov 4, 2024

@tool
extends EditorScript

func _run() -> void:
	var temp_variable452 = TextureProgressBar.new()
	
	var values : Array[float] = [360*1e4 + 10, - (360*1e4 + 10), INF, -INF]
	
	for val : float in values:
		temp_variable452.set_radial_initial_angle(val)
		print(val)
		print(temp_variable452.radial_initial_angle)
		print("---")

Output:

  Angle is non-finite.
  Angle is non-finite.
3600010.0
10.0
---
-3600010.0
350.0
---

This matches with how it worked before (except for infinite numbers).
Changing value from editor also works as before.

@arkology arkology force-pushed the to-infinity-and-beyond branch from 7cd187c to 21fa4d7 Compare November 4, 2024 16:29
@arkology
Copy link
Contributor Author

arkology commented Nov 4, 2024

Replaced fposmod with fposmodp, should work the same.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 5, 2024

I suppose it's not strictly required but adding a test about this would be nice.

@arkology
Copy link
Contributor Author

arkology commented Nov 5, 2024

I suppose it's not strictly required but adding a test about this would be nice.

Sure! I didn't touched Godot unit tests before but it will be definitely interesting to try.
I suppose this should be done in separate PR, right?

@Mickeon
Copy link
Contributor

Mickeon commented Nov 5, 2024

I suppose, yeah. The PR as is looks like an objective improvement.

@arkology
Copy link
Contributor Author

arkology commented Nov 7, 2024

@Mickeon test added - #98930

@arkology arkology changed the title Improve TextureProgress.set_radial_initial_angle() by removing loops Improve TextureProgressBar.set_radial_initial_angle() by removing loops Nov 7, 2024
@Mickeon Mickeon requested a review from KoBeWi November 7, 2024 19:27
@akien-mga
Copy link
Member

Sorry for the conflicting directives, but I think that adding the test in this PR would be best, so it can validate the change from this PR being correct.

@arkology
Copy link
Contributor Author

Good, I'll do it and close separate PR with test.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 12, 2024

I apologise for my own mistake!

@arkology
Copy link
Contributor Author

arkology commented Nov 12, 2024

@Mickeon don't worry, it didn't take much time to merge changes into single branch :)
Several minutes to verify that everything works as expected and I'll push changes.

@arkology arkology force-pushed the to-infinity-and-beyond branch from 21fa4d7 to 11034f4 Compare November 12, 2024 18:07
@arkology arkology requested a review from a team as a code owner November 12, 2024 18:07
@AThousandShips
Copy link
Member

Would be good to add test cases for non-finiteness as well

@arkology arkology force-pushed the to-infinity-and-beyond branch from 11034f4 to efbc643 Compare November 12, 2024 18:36
@arkology
Copy link
Contributor Author

arkology commented Nov 12, 2024

Would be good to add test cases for non-finiteness as well

Done.
UPD: Now for real :) Not only for INF, but also for NaN.

@arkology arkology force-pushed the to-infinity-and-beyond branch from efbc643 to 58a866d Compare November 12, 2024 18:46
@arkology arkology force-pushed the to-infinity-and-beyond branch from 58a866d to 0a4d48b Compare November 14, 2024 17:12
Replace two while loops with fposmodp.
Document radial_initial_angle wrapping.
Add testcases for set_radial_initial_angle()
@arkology arkology force-pushed the to-infinity-and-beyond branch from 0a4d48b to d692b7b Compare November 14, 2024 17:20
@arkology
Copy link
Contributor Author

I applied suggestions and rebased branch.

@Repiteo Repiteo merged commit d72112b into godotengine:master Nov 18, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 18, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextureProgress.set_radial_initial_angle(INF) freeze Godot
6 participants