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

move libs/** submodules into source tree #284

Merged
merged 30 commits into from
Jul 26, 2024
Merged

Conversation

sdumetz
Copy link
Contributor

@sdumetz sdumetz commented Jun 26, 2024

Hi,

This PR contains a lot of changes I wish to do to try to clean up duplicate/unused/unpractical code but most of those are mostly independant from each-other. It should be seen primarily as a pool of ideas, of which any number can be merged.

I tried to make readable commit messages but just to give a quick summary :

  • b518351..88ecb87: I moved everything that was in submodules under the libs/** folder to be checked-out in the repository.
    • I squashed each one individually to merge in the source tree. It would be possible to keep the entire commit history but that would make the log much more verbose. Here I think the original code attribution is clear and the history stays readable.
  • 115de38 remove untouched files automatically
  • 806fd52 remove dummy tests from the ff-* suite (left real tests untouched)
  • 30a5039 move everything under libs/**to source/**
    • I think this is more readable, more in line with how most projects manage their folder structure. Might be a matter of taste?
  • f764fb7 remove custom parseUrlParameter function
    • I don't like homegrown string parsers for such things when they can be avoided. QueryStrings are harder than they sound. Here it's not a security risk, but still. And less code.
  • 90cd054..558f960 remove all fetch.{text,json,file} references
    • It was mostly doing nothing more than fetch's defaults (which was already used in most place).
    • JSONWriter: Everything else is PUT in place without being routed through a writer, I couldn't see why JSON should be different. It makes CVAssetWriter more consistent.

git-subtree-dir: libs/ff-core
git-subtree-split: 301d02aacf4167a3527cc7d548de24f3850c5f57
git-subtree-dir: libs/ff-browser
git-subtree-split: 159a6e97f43497807674737c297696859309964a
git-subtree-dir: libs/ff-three
git-subtree-split: 6316e73d63b8b0eef03275e33b16053b8ff030c4
git-subtree-dir: libs/ff-ui
git-subtree-split: cf490da31e02dc4f92bfecedd32b22e622a41ed5
git-subtree-dir: libs/ff-graph
git-subtree-split: fda6c7a2a4b0f6133440752406f683372971b3d5
git-subtree-dir: libs/ff-scene
git-subtree-split: 196a5661e1df8799c74506dacaf88bc32a67166a
@sdumetz
Copy link
Contributor Author

sdumetz commented Jul 3, 2024

Working on the follow-ups for #288, I want to update three.js to r0.160+, which breaks down quite a lot of things from the ff-* libs. I've gone through the changelog and am ready to do the changes once we agree on a course of action on this.

@gjcope
Copy link
Collaborator

gjcope commented Jul 3, 2024

Do you have an example of another project that is managing folders like 30a5039? I'm all for moving the used code over, but I think the /lib organization makes it more obvious that these aren't Voyager libraries. But might feel otherwise if you have an example.

@sdumetz
Copy link
Contributor Author

sdumetz commented Jul 3, 2024

It depends if you think of the ff-* code as voyager's code (as nobody else uses it), in this case, gltf-transform comes to my mind, with some (more or less) internal packages and an external cli package.

If you don't consider them "Voyager code", it is better to show this and keep them as "vendors" in a separate folder.

Either way it's OK for me, I'll edit the PR to cut off the folder rename if you think it better reflects the ownership of this code.

@gjcope
Copy link
Collaborator

gjcope commented Jul 3, 2024

I don't consider the ff-* code as Voyager's code. They were developed outside of the project and I believe are (or at least were) used by its developer in other projects.

@sdumetz
Copy link
Contributor Author

sdumetz commented Jul 5, 2024

I dropped the commit that moved everything to source/.

@gjcope
Copy link
Collaborator

gjcope commented Jul 15, 2024

Working on merging this in now. Some of it seems like preference, but I'm ok with it. Still need to do a little testing, but the main issue I'm seeing right now is that a lot of the library cleanup you did doesn't seem to be coming through when I pull this branch. Something to do with removing that last commit?

There are also a few ff-* file updates for this next release so not sure those will get merged in appropriately...

@sdumetz
Copy link
Contributor Author

sdumetz commented Jul 15, 2024

Some of it seems like preference

Glad if you like it, there is clearly some subjective choices in here.

The submodules removal uses exotic merges that should come through cleanly but they might not. I don't know how a reference change introduced in a previous commit would get handled.

Do you have a working branch where I could get a look ? You just merged this branch over rc43?

@gjcope
Copy link
Collaborator

gjcope commented Jul 15, 2024

I hadn't yet because I was unsure how to merge the other ff-* library updates I made, but I can just redo them. I will merge with rc-43 tomorrow and let you know.

@gjcope
Copy link
Collaborator

gjcope commented Jul 16, 2024

https://github.com/Smithsonian/dpo-voyager/tree/dev-cleanup seems to somehow be missing the moving of the ff-* libs

@sdumetz
Copy link
Contributor Author

sdumetz commented Jul 18, 2024

I think I was able to reproduce, using "master", merging my branch fails with a bunch of "existing files would be removed".

Looks like you have to run git rm libs/ff-*, rm -rf libs/ff-*, commit and only then you can merge my branch.

@gjcope
Copy link
Collaborator

gjcope commented Jul 18, 2024

Ok, thanks. I'll give that a try locally. My usual release process is to merge the RC with Master through Github, but sounds like that may not work here. If so, I may do this commit last (or separately).

@gjcope gjcope merged commit 94a0de8 into Smithsonian:master Jul 26, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sdumetz FYI loadingManager support in getText() was commented out in release v0.43.0. The loading spinner is tied to the manager status and users found it annoying when navigating tours with articles that the spinner would flash every time and article came up. Would be great to maintain consistency but this needs a little more thought from a UX standpoint.

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