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

[glm] Bump to 2023-08-18 #34523

Closed
wants to merge 2 commits into from
Closed

Conversation

xiaozhuai
Copy link
Contributor

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

Fix GLM_FORCE_QUAT_DATA_XYZW not working.
See also #32685 (comment)

@dg0yt
Copy link
Contributor

dg0yt commented Oct 17, 2023

You suggest to use GLM_FORCE_QUAT_DATA_XYZW to enable old behaviour. However, it means that different downstreams could disagree on the actual data layout.
This problem seems to have existed before, but with the changed data layout, it becomes more relevant because downstream are forced to react.

It might have been easier if vcpkg wouldn't have diverged from the official release's default layout already.

I wonder if an extra paragraph in usage would help with this situation.

@MonicaLiu0311 MonicaLiu0311 self-assigned this Oct 17, 2023
@MonicaLiu0311 MonicaLiu0311 added the category:port-update The issue is with a library, which is requesting update new revision label Oct 17, 2023
@Osyotr
Copy link
Contributor

Osyotr commented Oct 17, 2023

It might have been easier if vcpkg didn't update glm at all. As a user, I would expect vcpkg install glm to install latest release, as any package manager would do.
Related: #34511

@xiaozhuai xiaozhuai marked this pull request as draft October 17, 2023 06:22
@xiaozhuai
Copy link
Contributor Author

Convert to draft. I would like to hear more views on this issue.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 17, 2023

@MonicaLiu0311 This might need a vcpkg team review.

@marekr
Copy link
Contributor

marekr commented Oct 17, 2023

It might have been easier if vcpkg wouldn't have diverged from the official release's default layout already.

Yes, vcpkg diverging is what has made this port difficult to use now.

vcpkg is now creating non-maintainer supported releases and there is zero documentation for these releases. Meaning, the API breaking changes are not documented. Such as the changing of the quanterion data structure.

You suggest to use GLM_FORCE_QUAT_DATA_XYZW to enable old behaviour. However, it means that different downstreams could disagree on the actual data layout.

Heh, we had to pin vcpkg glm to 0.9.9.8#2 :3

@MonicaLiu0311
Copy link
Contributor

@JavierMatosD Please help take a look at this PR.

There are different opinions on the impact of changes in GLM data layout and how vcpkg handles GLM version updates:
#34523 (comment)
#34511 (comment)

@JavierMatosD
Copy link
Contributor

JavierMatosD commented Oct 17, 2023

@JavierMatosD Please help take a look at this PR.

Can you explain how this update fixes the problem? From the conversation, it seems like vcpkg updated glm prematurely, diverging from upstream release schedule, which resulted in breaking changes to downstream users, i.e., the change to the data layout. This seems to be updating to a newer commit?

There are different opinions on the impact of changes in GLM data layout and how vcpkg handles GLM version updates: #34523 (comment) #34511 (comment)

I will consult with the team regarding how we handle GLM version updates.

@JavierMatosD JavierMatosD added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. info:reviewed Pull Request changes follow basic guidelines labels Oct 17, 2023
@xiaozhuai xiaozhuai marked this pull request as ready for review October 19, 2023 06:45
@MonicaLiu0311
Copy link
Contributor

Can you explain how this update fixes the problem? From the conversation, it seems like vcpkg updated glm prematurely, diverging from upstream release schedule, which resulted in breaking changes to downstream users, i.e., the change to the data layout. This seems to be updating to a newer commit?

The latest upstream release version is 0.9.9.8, released on 2020-04-14.

Macro GLM_FORCE_QUAT_DATA_WXYZ has been added upstream since 0.9.9.7: master/readme.md#improvements-2
Version 0.9.9.8 also retains this macro: 0.9.9.8/manual.md#section2_21

However, since 2021-06-01, the upstream has successively changed it to GLM_FORCE_QUAT_DATA_XYZW. But regarding these modifications, the upstream has not given corresponding instructions because they have not released a new version so far.
g-truc/glm@59ddeb7
g-truc/glm@f32eea1
g-truc/glm@9398c58
g-truc/glm@de7c83f

Prior to PR #32685, the version in VCPKG was consistent with upstream, but @xiaozhuai updated GLM to the latest commit(5c46b9c) at the time, which included these modifications from upstream and broke the data layout for some users.

Upstream made some fixes in g-truc/glm#1148, so @xiaozhuai started this update to make GLM_FORCE_QUAT_DATA_XYZW work. But whether the user's problem is really fixed may require @dsa-t's follow-up help to verify.

@xiaozhuai If you have any other clarifications, please add them, thanks.

@xiaozhuai
Copy link
Contributor Author

xiaozhuai commented Oct 19, 2023

Thanks @MonicaLiu0311
I think what you said is detailed enough.

In the meantime, I'll also explain why I submitted #32685

First of all, my projects did benifit from the update.
It's been more than 3 years since the last release of version 0.9.9.8.
Before latest update of this port, I tried to get in touch with upstream to get the new version released, but upstream didn't reply.

This leads us to believe that GLM is entering a rolling update phase, then I switch it to version-date and update to the latest commit at the time.

But unfortunately GLM introduced significant changes during this period, and that's something I haven't found.
Just as @marekr said, there is zero documentation.

My conclusion is:
If this broke the downstream user, I'd prefer to revert this port to 0.9.9.8. (It does happen at the moment).
Perhaps upstream will propose a good solution in a subsequent release, or improve the relevant documentation.

So I will close this PR.
Thanks @Osyotr for submiting #34591


Update:

What upstream said g-truc/glm#1069 (comment)

@xiaozhuai xiaozhuai closed this Oct 19, 2023
@MonicaLiu0311 MonicaLiu0311 removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 20, 2023
@xiaozhuai xiaozhuai deleted the dev-glm branch November 1, 2023 10:15
@xiaozhuai
Copy link
Contributor Author

@shrinktofit
Copy link

@dg0yt @marekr @MonicaLiu0311 @Osyotr @JavierMatosD Waiting for glm 1.0.0 See g-truc/glm#1186 g-truc/glm#1183 g-truc/glm#1180

Will glm 1.0.0 solve the problem? I'm just waiting for the fix for -glm::quat{1, 0, 0, 0} != glm::quat{-1, 0, 0, 0} under macro GLM_FORCE_QUAT_XYZW_ORDER.

@xiaozhuai
Copy link
Contributor Author

@dg0yt @marekr @MonicaLiu0311 @Osyotr @JavierMatosD Waiting for glm 1.0.0 See g-truc/glm#1186 g-truc/glm#1183 g-truc/glm#1180

Will glm 1.0.0 solve the problem? I'm just waiting for the fix for -glm::quat{1, 0, 0, 0} != glm::quat{-1, 0, 0, 0} under macro GLM_FORCE_QUAT_XYZW_ORDER.

GLM_FORCE_QUAT_XYZW_ORDER is already removed in g-truc/glm#1183.
Now the data layout is xyzw by default.
You can clone the master branch to test if the latest commits fix your problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants