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

Merging GLM migration work from freem #3026

Open
slipher opened this issue Jun 7, 2024 · 2 comments
Open

Merging GLM migration work from freem #3026

slipher opened this issue Jun 7, 2024 · 2 comments

Comments

@slipher
Copy link
Contributor

slipher commented Jun 7, 2024

This an issue to track integration of freem's work on GLM migration. The current statios is that sgame and shared (BG) are mostly migrated, but cgame is mostly not. freem suggests using his pre-release branch, rather than glm, because the GLM migration is further along there. It may have more GLM-unrelated changes, but on the other hand more commits from our side have been merged.

I am now trying a new strategy to incorporate the changes: instead of integrating the GLM migration commit by commit, integrate it function by function. Trying to cherry-pick commits would be awful because there are many merge conflicts and the Git conflict resolution process is too labor-intensive and error-prone. So instead I use a workflow of copying in one function at a time:

  • Load the version of some file from the tip of freem's branch into my editor
  • Copy and paste a function into the source, replacing the current definition
  • Undo diffs unrelated to GLM migration, such as unrelated refactorings or random whitespace changes.
  • Make any other needed adjustments for the function to build and run
  • Manually adjust callsites in other files (those diffs are usually too small to be worth copy-and-pasting)

I think it will work well, based on the trial in #3025.

@slipher
Copy link
Contributor Author

slipher commented Jun 7, 2024

In a discussion with freem we identified a couple of pitfalls when porting to GLM/merging from his branch:

  • If glm::normalize receives a zero vector, it produces undefined behavior/garbage values. VectorNormalize on the other hand leaves the zero vector unchanged.
  • freem is building with GLM 0.9.8, in which vectors have a default constructor that sets all components to 0. We are using GLM 0.9.9, in which the vector default constructor does no initialization. (Note that glm::vec3() or glm::vec3{} zeroes; only construction without any kind of braces leaves it uninitialized). So we have to be careful about merging bits with uninitialized vectors.

@slipher slipher changed the title Merging Merging GLM migration work from freem Jun 7, 2024
@illwieckz
Copy link
Member

illwieckz commented Jun 8, 2024

I actually think it's a good idea, and it would interfere less with rewrites like the ones to avoid UB:

On the other hand I like git history, and the ability to bisect.

The proposed workflow will not prevent bisecting, but what it will change is that the bisected branch would not be freem's one but a brand new one built from the results of freem's on.

For that task I will defend the way preferred by the ones willing to do the job, if it ensures the job will be done.

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

No branches or pull requests

2 participants