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

ThorVG: Update to 0.13.7 #92915

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

capnm
Copy link
Contributor

@capnm capnm commented Jun 8, 2024

@capnm capnm requested a review from a team as a code owner June 8, 2024 19:27
@AThousandShips AThousandShips added this to the 4.3 milestone Jun 9, 2024
@akien-mga akien-mga merged commit c568264 into godotengine:master Jun 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@@ -709,7 +709,7 @@ static bool _toColor(const char* str, uint8_t* r, uint8_t* g, uint8_t* b, char**
*ref = _idFromUrl((const char*)(str + 3));
return true;
} else if (len >= 10 && (str[0] == 'h' || str[0] == 'H') && (str[1] == 's' || str[1] == 'S') && (str[2] == 'l' || str[2] == 'L') && str[3] == '(' && str[len - 1] == ')') {
float th, ts, tb;
float_t th, ts, tb;
Copy link
Member

Choose a reason for hiding this comment

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

This broke the 32-bit builds again, fixed it up with 38716b9.

There's already a patch merged upstream to fix this, but if it doesn't make it to the next 0.13.x, we need to make sure to reapply the patch downstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This broke the 32-bit builds again, fixed it up with 38716b9.

There's already a patch merged upstream to fix this, but if it doesn't make it to the next 0.13.x, we need to make sure to reapply the patch downstream.

Could a Godot CI check be implemented for this issue so that everyone is alerted when similar ones occur?

Copy link
Member

Choose a reason for hiding this comment

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

Technically yes, but it's a matter of tradeoff between having a reasonably wide coverage on CI, and the amount of resources and time it takes for CI to pass on every single PR update.

Adding a 32-bit Linux or Windows build could be worth doing, but that's one more build to wait for many times a day.

Copy link
Member

@Calinou Calinou Jun 19, 2024

Choose a reason for hiding this comment

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

I'm surprised WebAssembly builds don't catch this, as WebAssembly is still 32-bit in its base specification. 64-bit WebAssembly exists but isn't widely used yet.

Copy link
Contributor Author

@capnm capnm Jun 19, 2024

Choose a reason for hiding this comment

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

I'm not sure if this is possible in github - we run an extended test suite daily, the daemon figures out who is to blame and sends them an alert.

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

Successfully merging this pull request may close these issues.

5 participants