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

Update xatlas to upstream 5571fc7, fixes #44017 #44023

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

Wavesonics
Copy link
Contributor

@Wavesonics Wavesonics commented Dec 1, 2020

Fixes a crash where the cross product's length is smaller than the epsilon value.

Fixes #44017.

@Wavesonics Wavesonics changed the title Possible fix for #44017 Possible fix for #44017 in master Dec 1, 2020
@Wavesonics
Copy link
Contributor Author

Wavesonics commented Dec 1, 2020

@Calinou I can change this to a .patch, but I'm not sure if this is the actual the answer. If anyone else has any knowledge of whats going on here I'd love some insight.

In the 3.x branch this fix works, the output can be opened in Godot and it looks fine. But in 4.0 it looks broken, at least the textures aren't loading correctly. Which I don't know if that is related to the original bug or not.

@Wavesonics
Copy link
Contributor Author

Wavesonics commented Dec 2, 2020

Ah okay, I think the real fix here is to take the latest from xatlas.

The normalize() function has been rewritten, now instead of using epsilon to check for non-zero, it instead simply checks that the length is non-negative. I tested this PR and it fixed my GLB import crash.

static Vector3 normalize(const Vector3 &v)
{
	const float l = length(v);
	XA_DEBUG_ASSERT(l > 0.0f); // Never negative.
	const Vector3 n = v * (1.0f / l);
	XA_DEBUG_ASSERT(isNormalized(n));
	return n;
}

instead of:

static Vector2 normalize(const Vector2 &v, float epsilon)
{
	float l = length(v);
	XA_DEBUG_ASSERT(!isZero(l, epsilon));
	XA_UNUSED(epsilon);
	Vector2 n = v * (1.0f / l);
	XA_DEBUG_ASSERT(isNormalized(n));
	return n;
}

This PR updates xatlas to 5571fc7ef0d06832947c0a935ccdcf083f7a9264, which is the current head of their master.

@Wavesonics Wavesonics marked this pull request as ready for review December 2, 2020 01:38
@Wavesonics Wavesonics changed the title Possible fix for #44017 in master Fix for #44017 in master by updating xatlas Dec 2, 2020
@fire
Copy link
Member

fire commented Dec 2, 2020

There's a reasonable assumption that we don't have to go through a regression test suite to test if things broke for xatlas, but we probably should.. Would like to merge for wider testing.

Copy link
Contributor Author

@Wavesonics Wavesonics left a comment

Choose a reason for hiding this comment

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

@fire if you wouldn't mind, there is one line of the change that I had to make that I didn't fully understand. I started a review of it. The function signature changed, and the ParameterizeOptions type was removed from xatlas. So I took a crack at getting it to compile just test my theory. But I don't totally understand what it was supposed to be doing on this line.


printf("Generate..\n");
xatlas::Generate(atlas, chart_options, xatlas::ParameterizeOptions(), pack_options);
xatlas::Generate(atlas, chart_options, xatlas::PackOptions());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone who knows more than me, I don't fully understand this change. It made it build, but someone should check my work?

Copy link
Member

Choose a reason for hiding this comment

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

Instead of using:


	pack_options.maxChartSize = 4096;
	pack_options.blockAlign = true;
	pack_options.padding = 1;
	pack_options.texelsPerUnit = 1.0 / p_texel_size;

You set it to the default. This is arguably a bug.

Copy link
Member

@fire fire Dec 2, 2020

Choose a reason for hiding this comment

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

You probably want xatlas::Generate(atlas, chart_options, pack_options);

@akien-mga
Copy link
Member

Could you amend the commit log to be more explicit about the change? It's true that your intent is to fix #44017, but what the commit does first and foremost is updating xatlas (which changes a lot of things, including a change that happens to fix #44017).

So a better commit log would be:

xatlas: Sync with upstream 5571fc7

Fixes #44017 by changing the `normalize()` function.

Please also update the commit hash for xatlas in thirdparty/README.md.

Fixes godotengine#44017 by changing the `normalize()` function to check for non-negative rather than non-zero via an epsilon check.
@Wavesonics
Copy link
Contributor Author

@akien-mga updated the commit log and the README

@akien-mga akien-mga added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:import topic:thirdparty labels Dec 2, 2020
@akien-mga akien-mga added this to the 4.0 milestone Dec 2, 2020
@akien-mga akien-mga changed the title Fix for #44017 in master by updating xatlas Update xatlas to upstream 5571fc7, fixes #44017 Dec 2, 2020
@akien-mga akien-mga merged commit f9b08be into godotengine:master Dec 2, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 2, 2020
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.

Crash on import of GLB
3 participants