Skip to content

Commit

Permalink
made assimp a submodule
Browse files Browse the repository at this point in the history
  • Loading branch information
@abma
abma committed Jul 8, 2011
1 parent 77424f1 commit 1bb82ca
Show file tree
Hide file tree
Showing 557 changed files with 4 additions and 199,801 deletions.
3 changes: 3 additions & 0 deletions .gitmodules
View file
Expand Up @@ -13,3 +13,6 @@
[submodule "AI/Skirmish/KAIK"]
path = AI/Skirmish/KAIK
url = git://github.com/Kaylewt/KAIK.git
[submodule "rts/lib/assimp"]
path = rts/lib/assimp
url = git://github.com/spring/assimp.git
1 change: 1 addition & 0 deletions rts/lib/assimp
Submodule assimp added at 350487
17 changes: 0 additions & 17 deletions rts/lib/assimp/CMakeLists.txt
View file

This file was deleted.

99 changes: 0 additions & 99 deletions rts/lib/assimp/CREDITS
View file

This file was deleted.

57 changes: 0 additions & 57 deletions rts/lib/assimp/INSTALL
View file

This file was deleted.

47 changes: 0 additions & 47 deletions rts/lib/assimp/LICENSE
View file

This file was deleted.

79 changes: 0 additions & 79 deletions rts/lib/assimp/README
View file

This file was deleted.

26 changes: 0 additions & 26 deletions rts/lib/assimp/README_SPRING
View file

This file was deleted.

15 comments on commit 1bb82ca

@hoijui
Copy link
Contributor

@hoijui hoijui commented on 1bb82ca Jul 8, 2011

Choose a reason for hiding this comment

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

YES WE CAN!
no really... why?

@tvo
Copy link
Member

@tvo tvo commented on 1bb82ca Jul 8, 2011

Choose a reason for hiding this comment

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

@hoijui: bit easier to upgrade assimp this way

@abma
Copy link
Contributor Author

@abma abma commented on 1bb82ca Jul 8, 2011

Choose a reason for hiding this comment

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

yes, actual i wanted to update assimp...

@hoijui
Copy link
Contributor

@hoijui hoijui commented on 1bb82ca Jul 9, 2011

Choose a reason for hiding this comment

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

that makes no sense; it is not easier to update.
submodules should be used for strictly optional stuff only.
the normal user wanting to compile spring from git should not have to use submodules. it would only complicate stuff (also for us, when we have to tell everyone and so on).
even if assimp is still optional (if it is, i do not know) it surely will/should not be in the long run, or say, as soon as games start to use formats supported by assimp.
submodularizing stuff also gives merge conflicts, always, so please thing 3 times next time you want to submodularize something, and maybe discuss it first.

@abma
Copy link
Contributor Author

@abma abma commented on 1bb82ca Jul 9, 2011

Choose a reason for hiding this comment

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

it is easier to update assimp: now you can exactly see what was modified. you can also make pull requests to merge changes upstream, before you couldn't.

both ways have drawbacks:

  • as submodule updates requires also to run "git submodule update"
  • when included, its really hard to update: you can't overwrite it with new files, that won't work as most files had modifications. now you can merge, and if luckily, you've no merge conflicts.
  • if we change/fix it ourself, updates would be impossible in future, so we should avoid this.

i see assimp as an external module, even if its currently not optional (but it shouldn't be hard to make it optional...)

we can't maintain assimp and it still contains many bugs, so i think its better to have it as git module. this also marks it as "please don't touch it, change it upstream"

(even if this change is really bad, "reverting" is very easy. most time i spent for this, was to make the newest version of assimp work)

@hoijui
Copy link
Contributor

@hoijui hoijui commented on 1bb82ca Jul 9, 2011

Choose a reason for hiding this comment

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

it should not be optional. that it is buggy is no reason to make it optional.
the AIs are fully optional, but as said, as soon as games will start to use assimp supported formats - and that is what we ultimately want and should prepare for now already - assimp will be anything else but optional.
when we make changes to assimp ourselfs, merging will not result in any more conflicts whether it is a submodule or not. the only difference is, that we would have to execute one or two very simple shell commands more (like rm -Rf rts/lib/assimp/*).
you treat a pro of very minor increased comfort for devs changeing assimp (which is a thing happening seldomly), against less comfort for anyone compiling spring, including main devs (i hardly ever use git submodule update, as i usually have local changes in the AIs, so i would run into conflict solving whenever rebasing on the main repo).

in short, the choice of whether to use a submodule or not does not depend on whether it exists as git repo, but purely whether it is optional or not.
assimp should not be a submodule.

@abma
Copy link
Contributor Author

@abma abma commented on 1bb82ca Jul 9, 2011

Choose a reason for hiding this comment

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

"merging will not result in any more conflicts whether it is a submodule or not"

hu? how will you merge, when its not a submodule? do a full directory-tree comparison?

(btw this would make it optional: http://pastebin.com/fdnazpkh only a few cmake changes would be still required...)

hm, i think you're right, but i dislike to do a ~300k code-commit again. talk about it on monday with others?

@hoijui
Copy link
Contributor

@hoijui hoijui commented on 1bb82ca Jul 9, 2011

Choose a reason for hiding this comment

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

yeah right, making a revert commit would not be optimal (still produces merge conflicts, maybe), i am in favor of force-push removing this change (cherry picking the commits after it).
maybe everyone should keep local changes local until we settled this (at the latest, monday evening) ... if you see this comment.

how to merge:

  1. check-out latest spring two times (dir master & assimp_changeX).
  2. cd assimp_changeX && git branch assimp_changeX && git checkout assimp_changeX
  3. rm -Rf rts/lib/assimp/*
  4. now get latest assimp under rts/lib/assimp/
  5. commit the change
  6. cd ../master
  7. git merge ../assimp_changeX/assimp_changeX
  8. git push origin master

of course we would not have assimps own development history this way, but we do not need that. we could create an additional file in step 4.b, containing the assimp SHA1 or something, for easy reference.

@tvo
Copy link
Member

@tvo tvo commented on 1bb82ca Jul 9, 2011

Choose a reason for hiding this comment

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

Good points hoijui.

I should have considered it a bit longer before saying it's fine with me to abma :-)

@jk3064
Copy link
Contributor

@jk3064 jk3064 commented on 1bb82ca Jul 10, 2011

Choose a reason for hiding this comment

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

yeah, I am skeptical of submodules here, too.
On the other hand, I see the simplification of updating.
Isn't there a way to force submodule checkouts/syncs in "git clone" & "git pull"?

@hoijui
Copy link
Contributor

@hoijui hoijui commented on 1bb82ca Jul 10, 2011

Choose a reason for hiding this comment

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

no, there is no way to do that purely from the repos side.

btw, the "how to merge" instructions i gave above are wrong. they would always overwrite our own changes to assimp.
we need an assimpUpstream branch, and then the procedure works like this:

  1. remove the submodule
  2. git branch assimpUpstream && git checkout assimpUpstream
  3. a) get assimp stuff under rts/lib/assimp/
    b) [optional] put the output of assimps "git describe" into rts/lib/assimp/VERSION or the like
  4. commit (at best, also put "git describe" output from assimp into the commit message)
  5. git checkout master
  6. git merge --no-ff assimpUpstream
  7. git push origin master
  8. git push origin assimpUpstream

from now on, always update assimp version in the assimpUpstream branch.
the same technique should/could be done for rts/lib/lua, i guess (so we could profit from gits merge magic there too).

@tvo
Copy link
Member

@tvo tvo commented on 1bb82ca Jul 10, 2011

Choose a reason for hiding this comment

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

Even if it is not a submodule, we could use the spring/assimp repository to maintain our changes and easily merge upstream / send pull requests as Spring team, and then instead of a submodule update do the rm -rf rts/lib/assimp & git clone git@github.com:spring/assimp.git etc. in the Spring repository.

That way our changes to assimp remain clear as a patch set in spring/assimp/master (as long as we rebase & force push when we update, instead of doing a real merge), there aren't any of the disadvantages of a submodule, and updating assimp can be done more easily (by having git rebase handle any merge conflicts per commit, etc.)

@hoijui
Copy link
Contributor

@hoijui hoijui commented on 1bb82ca Jul 10, 2011

Choose a reason for hiding this comment

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

ahh i see! true .. even better that! :-)
i don't get why we would need to force push anywhere in this scenario, but maybe you can explain this next time in chat.

i will possibly not be around in tomorrows meeting (Monday 10. July 2011).

@tvo
Copy link
Member

@tvo tvo commented on 1bb82ca Jul 10, 2011

Choose a reason for hiding this comment

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

@hoijui, to keep the spring changes from drowning under hundreds of new commits in spring/assimp I think we'll want to git pull --rebase when merging upstream. And that implies a force push will be needed to push that update.

@hoijui
Copy link
Contributor

@hoijui hoijui commented on 1bb82ca Jul 10, 2011

Choose a reason for hiding this comment

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

ahhh i see, thanks for explanation. :-)
makes sense.

Please sign in to comment.