Skip to content

Conversation

@toji
Copy link
Owner

@toji toji commented Aug 8, 2025

This work has been outstanding for a while, and I haven't moved as proactively as I should have to accept it. Taking the initiative to put up a PR here (and resolve any merge issues that come up) in hopes that @typhonrt is still willing to make the code contribution.

@toji
Copy link
Owner Author

toji commented Aug 8, 2025

Gonna take me a moment to figure out how to resolve the merge conflicts from this end. Might end up in this PR getting dropped in favor of another approach, but wanted to indicate that I was working on it.

@typhonrt
Copy link

typhonrt commented Aug 8, 2025

Yeah.. I saw the movement on 3.4.4. Glad that there still is interest to upstream. The challenges with the additional upstream commits given that I have significantly refactored the codebase is a bit dicey in terms of an easy merge through normal procedures. There has been 1 relatively low impact commits upstream with just 2 changing a lot of the signatures (templating / Aiden Foxx PRs committed in March), but not easy merges due to the refactoring downstream. There is a reason I tried to reach out to you privately (email) in December when I saw you make that first commit upstream, but alas did not hear back, so the additional 2 commits in March are more significant changes, but just signatures / templating for the most part.

image

It will be very time consuming on my side to try to resolve the upstream commits in my downstream fork given the large scale refactoring. I think there are two reasonable options:

  • (preferred): rollback the 3 commits upstream, allows a merge from downstream, then just reapply the code changes from those 3 commits in a follow up PR.

  • hard reset the upstream / main repo with my downstream fork and then reapply the 3 upstream commits.


For the preferred approach of just reverting the 3 commits it should be this easy:

git revert <commit-sha1>
git revert <commit-sha2>
git revert <commit-sha3>

Of course take a snapshot of the code changes in those commits just to make it easy to make the changes once the merge completes. I can assist in this process.

I have already grabbed a copy of the current state of upstream gl-matrix#glmatrix-next branch.

@toji
Copy link
Owner Author

toji commented Aug 8, 2025

I don't want to take any more of your time than necessary because I merged a few PRs ahead of yours, so I did the rebase here: https://github.com/toji/gl-matrix/tree/typhonrt-merge 😁 It's not clear if there's any sane way to merge that into glmatrix-next, so I may simply delete the glmatrix-next branch and rename that one into it's place.

Assuming you approve of the merge, of course! And I do sincerely apologize for leaving this hanging for so long. It's been a challenge to keep up with projects like this when work and family demands are high.

@typhonrt
Copy link

typhonrt commented Aug 8, 2025

I don't want to take any more of your time than necessary because I merged a few PRs ahead of yours, so I did the rebase here: https://github.com/toji/gl-matrix/tree/typhonrt-merge 😁 It's not clear if there's any sane way to merge that into glmatrix-next, so I may simply delete the glmatrix-next branch and rename that one into it's place.

Yeah.. Always multiple ways to proverbially "skin a cat". As the maintainer choosing whatever you are most comfortable with is reasonable. From what I gather the typhonrt-merge branch will become the main 4.x branch and then those 3 commits just need to reapplied / added to the typhonrt-merge branch manually. It doesn't look like that will be too bad.

Assuming you approve of the merge, of course! And I do sincerely apologize for leaving this hanging for so long. It's been a challenge to keep up with projects like this when work and family demands are high.

Indeed.. I'm quite behind where I'd like to be w/ the various work I have on my plate as well currently. Definitely glad to just have things move forward w/ the 4.0 glmatrix release; long time coming!

I'm just a tad behind on updating the documentation tooling to the latest TypeDoc, so can certainly swing around very soon (~2-4 weeks) to provide an update there as well as help with any ancillary aspects in the gap since I last put effort into the fork. Everything should still work w/ the current package dependencies set.

I've been shipping the fork in my framework this entire time.

@toji
Copy link
Owner Author

toji commented Aug 9, 2025

From what I gather the typhonrt-merge branch will become the main 4.x branch and then those 3 commits just need to reapplied / added to the typhonrt-merge branch manually. It doesn't look like that will be too bad.

It's already done, actually! If you look at the commit log for that branch it's already had the 3 commits rebased onto it. A bit of a pain but it wasn't too hard. Just monotonous.

I'm just a tad behind on updating the documentation tooling to the latest TypeDoc, so can certainly swing around very soon (~2-4 weeks) to provide an update there as well as help with any ancillary aspects in the gap since I last put effort into the fork. Everything should still work w/ the current package dependencies set.

No worries! Those can be worked on over time. I don't view it as release blocking.

I've been shipping the fork in my framework this entire time.

Good to hear! It's comforting to know that the codebase has seen some real-world use.

@toji
Copy link
Owner Author

toji commented Aug 11, 2025

@typhonrt: Okay, after toying around with this branch for the weekend in some of my own projects I've got a couple of tweaks to the dist files I'd like to make prior to pushing this as beta 3.

The primary speedbump I hit was that if I want to use the library in browser as an ES module without using a bundler then my only option appears to be the dist-cdn/esm bundle, which feels a bit odd. dist/index.js is intended to be an ES module but it is not, due to the fact that that the import it uses doesn't include the file extension and relative file path.

Specifically it currently includes the line

import { GLM_EPSILON } from "gl-matrix/common";

instead of the browser-compatible

import { GLM_EPSILON } from "./common.js";

I'd really like to see a version of the library in the dist folder that can be imported directly by a script running in-browser.

(Technically the first one CAN be used in the browser if you add a imports table to the HTML file, but in practice this is pretty aggravating to use effectively and there's no reason to require it when a relative file path will work just as well.)

A secondary hitch that I found was that previous releases of the library have included options for importing only a single file if that's all you need. (ie: import { Mat4 } from "./glmatrix/Mat4.js";) While a reasonable number of users won't need to care about this due to tree-shaking build steps I'd still like to include the option to use individual file for at least the esm build, where users are most likely to try and use these files directly without a build step.

I can work on enabling both of these on top of the build structure you've put in place, but wanted to bounce it off of you real quick in case you had any concerns or knew of easy way to enable those outputs.

Thanks again for your work here and your patience!

@typhonrt
Copy link

I'd really like to see a version of the library in the dist folder that can be imported directly by a script running in-browser.

Either you adopt modern mainstream distribution or you don't. What you are asking for is an unbundled / non-subpath separated version of the library which will inherently negate modern mainstream distribution benefits as currently configured while also re-introducing a less maintainable local distribution in respect to the f32 / f64 code generation.

For lightweight adhoc browser demo code using a bundle of choice ala dist-cdn/esm/2022/gl-matrix-f32.js is very possible. There is no benefit for adhoc browser demo code to use an unbundled version of the library. The NPM built distribution (dist/) which is not checked into the repo requires consumers to have Node / NPM installed and run the build process before it is even available in a local copy of the repo. Consumers can directly download the checked in dist-cdn resources from the repo and just use the one of choice (CJS, ESM, UMD). Or when actually published simply include it from one of many JS / NPM mirroring CDNs without even downloading anything; IE simply reference a script w/ the CDN link.

You can hack the configuration and attempt to satisfy your own historical / personal preference for lightweight unbundled demo usage, but quite likely in the process compromise the modern distribution benefits currently realized. At the end of the day this is not my repository and I have done the best I can providing my take on an informed modern distribution of glmatrix that also fully serves the long tail and then some. About the only thing that isn't maintained is a raw unbundled distribution.

I gather you are just not aware of the benefits currently available that you haven't used personally yet. I also can't spend more time on attempting to satisfy this particular request as I believe I have fulfilled the requirements necessary for a modernized glmatrix distribution. If you do decide to make further changes I can spend a small amount of time to at least try to verify if you happen to break any of the current modern distribution features / types / etc.

@typhonrt
Copy link

typhonrt commented Aug 12, 2025

@toji I know the above is a bit of push back, but there are good reasons for this. I should also state not everyone's idea of clean code is the same. To create an unbundled raw distribution of code this requires duplication of all source code even shared source code between the F32 / F64 variations or make that duplication process more complicated than it needs to be. The common / utility code is separated by using sub-path exports where those specific sub-path exports can simply be referenced between the variations. This makes the F64 duplication script significantly simpler and easier to maintain. There is also guaranteed downstream tree-shaking when combining the F32 / F64 variations in the same project. Only one copy of common / util code will be included when using both variations. Furthermore it makes the API docs clear and well separated and the API docs are generated from the types which also are cleanly generated.

The built /dist resources is the NPM distribution and it very much is ESM, but glmatrix/common is just a sub-path export. It is 100% valid within the Node consumption of the package. You can even use import maps in the browser if desired to define what glmatrix/common points to, but that is not really necessary and not a mainline use case, but it should be referenced in the documentation how to do it. You can directly copy the NPM package output and easily use it in an adhoc project; just not in the bare / raw way that you may have done in the past.

In the past you may have just copied raw unbundled source of glmatrix to a project, but how many other libraries / NPM packages do you do this for in your personal demo projects? This is not a common consumption practice. For easy consumption for CDN use cases and targeting the long tail through transpilation whether CJS / UMD or even ES2016 in the case for ESM various bundles are created. Through clear documentation IMHO it is fine to direct consumers that wish to not use modern build tooling to copy the bundle of choice to their local project or as mentioned provide the CDN mirrored links of the bundled package after being published.

I'd be glad to answer any questions you have about design decisions and structure of the codebase. I do use these newer mechanisms of Node / NPM all the time in my framework for at least 5 years now.

@toji
Copy link
Owner Author

toji commented Aug 12, 2025

Sorry, I've had a partial response typed up but I've come down with something the last couple of days and didn't want to be trying to work this out with a foggy head. I do have some questions about how things are structured, some of which I think you've already answered in past posts or videos and I didn't want to make you repeat yourself, but it's probably best just to ask again.

Overall I think it's in a good place, I just want to have a slightly clearer path for a certain class of beginner/casual user. Give me another day or so to get over whatever bug I've got and I'll probably be able to communicate far more clearly. 😅

@toji
Copy link
Owner Author

toji commented Aug 13, 2025

Okay, feeling significantly less brain-foggy than I was for the last couple of days! So let me get a couple of questions out of the way. None of these are requests for changes, it's just helping my own understanding of the code/build layout since I'll be maintaining it.

  • Which of the outputs did you intend for publication to NPM for this package? I'm assuming the output in dist folder, but there's also several CDNs that simply mirror NPM packages, so perhaps the dist-cdn/ should be included?
  • Why are the dist-cdn/ files checked into the repo, while dist/ isn't?
  • What's the reason behind separating out common/ and util/ in the build output? I think it's fine that they're separate in the code base, but the fact that those are the only two bits of code that don't get bundled indicates that it's intentional and I'm not sure I understand why?

Aside from that, I should also mention that in my merge branch I've removed the swizzles code. I know you tried to make them work as elegantly as possible with the code base, but at the end of the day it seemed like too much added complexity for not enough benefit. Might work better as a complimentary but separate library. In any case, I can worry about that later.

@typhonrt
Copy link

None of these are requests for changes, it's just helping my own understanding of the code/build layout since I'll be maintaining it.

Indeed you caught me on a busy week as I hunkered down to do a significant release of my framework on Saturday / past weekend. I'm glad to answer any questions. Also I should state that I'll be around to help w/ any ongoing maintenance. I don't foresee switching matrix libraries anytime soon.

  • Which of the outputs did you intend for publication to NPM for this package? I'm assuming the output in dist folder, but there's also several CDNs that simply mirror NPM packages, so perhaps the dist-cdn/ should be included?

The dist/ folder is the main NPM distributable asset. dist-cdn/ is included as well as you can see in the package.json files section. Both will get picked up by the JS / NPM CDNs. The CDN bundles also have specific sub-path exports available in the main NPM package.

  • Why are the dist-cdn/ files checked into the repo, while dist/ isn't?

Typically one doesn't check-in the distribution resources for a NPM package that is published as it is just "extra noise" in the repo. The reason the dist-cdn/ complete standalone bundles are checked in are so folks can download / grab one of these resources directly from the repo without having to install Node / build the project to access the resource which is more steps for a new user or those not dealing w/ the Node / NPM ecosystem.

The only time it makes sense to check in dist/ is if it is a non-published package and you are pulling in the package through Github links vs using NPM. This for instance can be seen in the additional "distribution" fork I made of the refactor fork explicitly to just include dist/ in the checked-in repo so it can be pulled in from a Github link. I'm consuming my gl-matrix fork in my framework this way. This also was referenced in the main discussion thread on how folks can test out my work which was unpublished on NPM, but accessible still.

  • What's the reason behind separating out common/ and util/ in the build output? I think it's fine that they're separate in the code base, but the fact that those are the only two bits of code that don't get bundled indicates that it's intentional and I'm not sure I understand why?

They are shared resources across the f32 / f64 variations. There is no code duplication required of these resources. It makes the code generation of the f64 variation simple without potentially having to change import paths. It also is a future proof boundary setup and ready to go for any additional shared resources between the variations whether they fit in common code or utility code.

Also when downstream consumption occurs where both f32 / f64 variations are utilized tree-shaking is automatic / no possible duplication of common / util code.

Typically consumers don't need to access these sub-paths, but they can to just access to util code or perhaps use the same constant from common that gl-matrix does internally.

Aside from that, I should also mention that in my merge branch I've removed the swizzles code. I know you tried to make them work as elegantly as possible with the code base, but at the end of the day it seemed like too much added complexity for not enough benefit. Might work better as a complimentary but separate library. In any case, I can worry about that later.

The only push back I'll provide here especially since I won't ever use the swizzle path myself is that it was fully working as intended including top notch types support that only is engaged when a consumer utilizes the swizzle setup. IE it 100% worked and worked well w/ types support across all of the distribution mechanisms. To me this seems more like a documentation concern in regard to the API docs being overcomplicated / lots of extra data for a sideline use case. Since the engineering aspects are sound for this feature and if the actual concern is documentation oriented or a concern around perceived complexity of the feature by consumers of the library I would explore excluding the additional swizzle API from the mainline API docs and just make an addendum / markdown file that documents the swizzle API before wholesale removing the feature.

I don't know how many consumers use this feature or if it will be missed. I do agree that it does add a lot of perceived noise to the API docs when included in the mainline docs. I also agree that libraries should be lean and mean and focus on mainline use cases. It just happens that the swizzle engineering / types effort is feature complete though, so I'm curious if the source of your decision had engineering / maintenance concerns or was just driven by documentation overhead / consumer perception.

@typhonrt
Copy link

typhonrt commented Nov 4, 2025

Will there be any further movement on this PR / issue? Glad to help & resolve any remaining understanding or updates necessary.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants