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

[no squash] Irrlicht import #14483

Merged
merged 3 commits into from Mar 26, 2024
Merged

[no squash] Irrlicht import #14483

merged 3 commits into from Mar 26, 2024

Conversation

Desour
Copy link
Member

@Desour Desour commented Mar 21, 2024

First commit just copies irrlichtmt (as of minetest/irrlicht@1247087) to <root>/irr/.

Second commit makes use of this irrlichtmt version, and removes a bunch of now useless stuff.

Related: #13642

Note: lib/irrlichtmt is still gitignored. This is good because you can still bisect this way, and also we don't mess with repos lying around there.
Users can decide themselves when they want to delete it, but they have to do it manually.

Co-authored by sfan5.

To do

This PR is Ready for Review.

How to test

  • Build.
  • Watch the CI.
  • Check and test build artifacts.
  • Read through other commits. Reproduce 1st commit if you want.

@Desour Desour added @ Build CMake, build scripts, official builds, compiler and linker errors @ Engine Core What happens inside the very engine labels Mar 21, 2024
@Desour Desour added this to the 5.9.0 milestone Mar 21, 2024
@Desour Desour force-pushed the irrlicht_import branch 2 times, most recently from dbd6d34 to aa91d0a Compare March 21, 2024 20:15
@Desour Desour marked this pull request as draft March 21, 2024 20:17
@Desour
Copy link
Member Author

Desour commented Mar 21, 2024

The CI on ubuntu 20.04 is failing because of SDL2, see also:
https://github.com/minetest/irrlicht/blob/05c8bc831485077f7fffe05b186936928249de75/.github/workflows/build.yml#L64

And the mingw stuff downloads dependencies from @sfan5's website.

I need help here.

@sfan5
Copy link
Member

sfan5 commented Mar 21, 2024

one more: please amend the import commit with an author like import <script@mt> so the commit stats don't get thrown way off.

@sfan5 sfan5 added the Roadmap The change matches an item on the current roadmap. label Mar 21, 2024
@appgurueu
Copy link
Contributor

Perhaps I'm missing something, but 60b1249 seems to just copy the sources? This would lose history entirely, as opposed to a subtree merge which would preserve history.

@sfan5
Copy link
Member

sfan5 commented Mar 21, 2024

My plan never was to do a subtree merge.

@Desour
Copy link
Member Author

Desour commented Mar 21, 2024

Perhaps I'm missing something, but 60b1249 seems to just copy the sources? This would lose history entirely, as opposed to a subtree merge which would preserve history.

Yes, it's just a copy. AFAIK, this is what we planned to do.

one more: please amend the import commit with an author like import <script@mt> so the commit stats don't get thrown way off.

📈 🚫
Will do after CI ran through.

import and others added 2 commits March 21, 2024 23:26
Also remove the now useless options (like IRRLICHT_INCLUDE_DIR)
and update download instructions, CI and similar.

Co-authored-by: sfan5 <sfan5@live.de>
@Desour Desour marked this pull request as ready for review March 21, 2024 22:28
@Desour
Copy link
Member Author

Desour commented Mar 21, 2024

Re-copied from current irrlichtmt master, and squashed.

please amend the import commit with an author like import <script@mt>

This is done, btw.. Github still shows my profile picture because I'm the committer, and it doesn't know the import <script@mt> author.

@nerzhul
Copy link
Member

nerzhul commented Mar 22, 2024

cannot we import this properly in git mode keeping irrlicht mt commits ? there is a way to merge repo

@appgurueu
Copy link
Contributor

cannot we import this properly in git mode keeping irrlicht mt commits ? there is a way to merge repo

Yes, that's what I'm inquiring about: Why we aren't doing a subtree merge.

There may be benefits to not doing a subtree merge, but I don't really see them yet (besides a slightly smaller repo, maybe). I think much of the Irrlicht(MT) history is probably still useful.

@sfan5
Copy link
Member

sfan5 commented Mar 22, 2024

Reasons for not doing a subtree merge (IMO):

  • irrlicht svn history is only partial anyway
  • this pulls in lots of changes because we just reformatted and moved the code
    • also the CRLF conversion and the many thousands lines of code we deleted over the years
  • irrlicht repo can be kept around as archive for a while
  • not sure if that creates a merge commit, we like to avoid those

For a comparison consider the clone size of irrlicht:

full:
Receiving objects: 100% (12904/12904), 23.20 MiB | 12.03 MiB/s, done.
shallow:
Receiving objects: 100% (368/368), 875.69 KiB | 7.12 MiB/s, done.

@Desour
Copy link
Member Author

Desour commented Mar 23, 2024

not sure if that creates a merge commit, we like to avoid those

And if it doesn't, it would instead add around 500 commits on top of our history.

I also prefer the current choice of just copying it in.

(Self-approving.)

@ryvnf
Copy link
Contributor

ryvnf commented Mar 24, 2024

I do not think its a good idea to import this without git subtree. It just feels completely unnecessarily to throw away the git history and make useful commands like git blame and git bisect useless. 500 commits is also barely anything and you can always use --depth when cloning.

I recognize this is not up to me but it feels a bit questionable.

@lhofhansl
Copy link
Contributor

I think we want to get rid of as much of Irrlicht as we can. The history is less and less important. With all the changes we did, chances are we cannot integrate anything from/to upstream anyway - and that is assuming that Irrlicht upstream isn't mostly dead anyway.

Count as in favor of just copying the files over (not history) - and then remove as much as we can of Irrlicht over time.

@Desour
Copy link
Member Author

Desour commented Mar 25, 2024

make useful commands like git blame and git bisect useless.

For git bisect, you need to clone irrlichtmt the old way into lib/ anyway. (And you can't bisect only irrlichtmt, because it only works with the corresponding minetest version.)

git blame would in many cases hit on a barrier anyways, because of the reformatting and source file moves. Anyways, you can just continue the blame in the irrlichtmt repo when you hit the import commit.

@lhofhansl
Copy link
Contributor

Can we move ahead with this, or is there further discussion to have?

@celeron55
Copy link
Member

The goal of forking Irrlicht was to fuse Minetest and irrlicht together to enable customizing the entire rendering system to suit MT's special use case which will never be Irrlicht's goal in its upstream.

It doesn't seem to me like a bad time to do the repo import now - irrlichtmt is relatively problem free and has shrunk a lot from its original size. While we have had a very fluctuating availability of graphics programmers to improve, optimize and modernize the rendering, there has been enough progress to validate the sanity of the plan.

Part of this is removing everything unneeded from Irrlicht to make it manageable and appetizing for new development. Irrlicht contains vast amounts of stuff that is of no use and no interest to us, and is only a burden. Not including the history of irrlichtmt in the minetest repo fits all this well enough.

@grorp
Copy link
Member

grorp commented Mar 26, 2024

Re opinions needed: I like this 👍

@sfan5 sfan5 merged commit 6a7a613 into minetest:master Mar 26, 2024
15 checks passed
@Desour Desour deleted the irrlicht_import branch March 26, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Build CMake, build scripts, official builds, compiler and linker errors @ Engine Core What happens inside the very engine Roadmap The change matches an item on the current roadmap. >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants