go-gl-legacy / gl

Go bindings for OpenGL
BSD 3-Clause "New" or "Revised" License
342 stars 52 forks source link

fix ClipPlane; change type of equation arg from *float64 to []float64 #158

Closed tildeleb closed 10 years ago

tildeleb commented 10 years ago

fix ClipPlane; change type of equation arg from *float64 to []float64

pwaller commented 10 years ago

Could you please rebase your change against go-gl/master? If it's not clear how to do that I'll lend a hand.

tildeleb commented 10 years ago

I am not much of git hacker yet as I work alone most of the time. I think the mistake I made is that I did fetch/merge at the same time as I did the two line change for ClipPlane and what I should have done was fetch/merge/commit first and then a separate commit for my simple change?

I don't think I can rebase because my change was mixed in with the fetch/merge changes?

I am not sure I know how to fix this without deleting my fork and making a new fork, which I am OK to do unless you have a better way?

Sorry

leb

On Jul 1, 2014, at 5:37 AM, Peter Waller notifications@github.com wrote:

Could you please rebase your change against go-gl/master? If it's not clear how to do that I'll lend a hand.

— Reply to this email directly or view it on GitHub.

Lawrence E. Bakst +1-408-930-3801 (mobile)

dmitshur commented 10 years ago

I highly recommend this article as a primer, if you have the time: http://www.jayway.com/2013/03/03/git-is-a-purely-functional-data-structure/. That article, IMO, will do a much better job helping you understand how rebases work than any other.

Since you've made the change on master of your own fork, I think you would need to add go-gl as a second remote, then rebase against its master (something like git rebase upstream/master).

This stackoverflow question is relevant: http://stackoverflow.com/questions/17182624/contributing-to-project-on-github-how-to-rebase-my-pull-request-on-top-of-mast

Or, in this particular case, it would be faster just to delete your fork and redo the commit, since it's only 2 lines. But you'll learn less for next time. Your call.

tildeleb commented 10 years ago

Thanks, I'll read the article and try to figure out how to do the rebase. Appreciate your tips too. Actually I think I read the article before because I saw it on hacker news or someplace like that. I believe I understand what rebase does conceptually but I just don't have any practical experience using it.

leb

On Jul 2, 2014, at 1:15 AM, Dmitri Shuralyov notifications@github.com wrote:

I highly recommend this article as a primer, if you have the time: http://www.jayway.com/2013/03/03/git-is-a-purely-functional-data-structure/. That article, IMO, will do a much better job helping you understand how rebases work than any other.

Since you've made the change on master of your own fork, I think you would need to add go-gl as a second remote, then rebase against its master (something like git rebase upstream master).

Or, in this particular case, it would be faster just to delete your fork and redo the commit, since it's only 2 lines. But you'll learn less for next time. Your call.

— Reply to this email directly or view it on GitHub.

Lawrence E. Bakst +1-408-930-3801 (mobile)

dmitshur commented 10 years ago

I think the best way to learn is to try it. You can always make a full backup of your (local) git repo just by copying it, so if something goes wrong, just try again from scratch.

The way I've learned to use git is just to understand how it works conceptually, then when I need to do something new for the first time, I look up the commands, try them, and if it works, add to a .txt file of "known good commands that I've used in the past".

When it comes to doing a git rebase, it's pretty straightforward because if you read the messages, it tells you exactly what to do. If you're not sure, do git status and it will tell you.

pwaller commented 10 years ago

Assuming you're on your master branch, and you haven't made commits since you last pushed, you can:

git fetch git rebase go-gl/master git push your-remote HEAD --force

The only question is what go-gl and your-remote are called. You can find out by running git remote -v. If you cloned from your fork, then your-remote will be called origin, and you'll see something that looks like:

origin  git@github.com:tildeleb/gl (fetch)
origin  git@github.com:tildeleb/gl (push)

If you only see origin listed there, you need to add go-gl, which you can do by running git remote add git://github.com/go-gl/gl. You'll then need to pull down the content from this new remote by saying git fetch go-gl, at which point git rebase go-gl/master will work.

tildeleb commented 10 years ago

Short version: I made a new fork and resubmitted the change.

tl;dr I appreciate everyone's help with git. My repo was kind of messed up because it was a clone of the main repo and not a clone of my github fork. Actually I don't think that should have made a difference. I tried doing the fetch/rebase/push anyway but with the origin and your-re,ote reversed however it got some kind of fatal error:

leb@hula:~/gotest/src/github.com/go-gl/gl % git fetch leb@hula:~/gotest/src/github.com/go-gl/gl % git rebase origin/master First, rewinding head to replay your work on top of it... fatal: Not a valid object name From 88d7f366486dbff378f2be778acf44ff91d6041c Mon Sep 17 00:00:00 2001 Applying: added support for glGetActiveAttrib Using index info to reconstruct a base tree... M program.go Falling back to patching base and 3-way merge... No changes -- Patch already applied. fatal: Not a valid object name From 3daa9166f1ae6907af6ecd4a3a8f2343e531d08c Mon Sep 17 00:00:00 2001 Applying: fix ClipPlane; change type of equation arg from *float64 to []float64 leb@hula:~/gotest/src/github.com/go-gl/gl % git push leb HEAD --force

Maybe program.go got modified by accident as I am working on some shader stuff anyway and had it open in my editor.

Anyway thanks again to Peter and Dmitri.

leb

On Jul 2, 2014, at 6:51 AM, Peter Waller notifications@github.com wrote:

Assuming you're on your master branch, and you haven't made commits since you last pushed, you can:

git fetch git rebase go-gl/master git push your-remote HEAD --force

The only question is what go-gl and your-remote are called. You can find out by running git remote -v. If you cloned from your fork, then your-remote will be called origin, and you'll see something that looks like:

origin git@github.com:tildeleb/gl (fetch) origin git@github.com:tildeleb/gl (push) If you only see origin listed there, you need to add go-gl, which you can do by running git remote add git://github.com/go-gl/gl. You'll then need to pull down the content from this new remote by saying git fetch go-gl, at which point git rebase go-gl/master will work.

— Reply to this email directly or view it on GitHub.

dmitshur commented 10 years ago

I tried doing the fetch/rebase/push anyway but with the origin and your-re,ote reversed however it got some kind of fatal error

Nevertheless, it seems to have worked, if you look at https://github.com/go-gl/gl/pull/158/commits there's only one good commit there. In fact, this PR looks better than #159.

dmitshur commented 10 years ago

LGTM, I think it can be merged.

dmitshur commented 10 years ago

I don't feel comfortable merging this myself, because I don't actively use this package, so it's hard for me to test it.

tildeleb commented 10 years ago
  1. FWIW it can't be much more broken than what's currently there.
  2. I have testing it and I am using ClipPlane to "cut" various types of geometry.

I think the biggest reason not to accept the change is because it uses a slice instead of a fixed size array.

As pointed out in my original problem, there are some bigger issues here having to with slices vs arrays going forward. I am going to make a proposal for that soon. If you want to hold the merge that's fine with me.

leb

On Jul 5, 2014, at 7:08 PM, Dmitri Shuralyov notifications@github.com wrote:

I don't feel comfortable merging this myself, because I don't actively use this package, so it's hard for me to test it.

— Reply to this email directly or view it on GitHub.

Lawrence E. Bakst +1-408-930-3801 (mobile)

dmitshur commented 10 years ago

I already gave this PR a LGTM, so another maintainer can merge if they also feel it looks good to them.

You should bug someone from https://github.com/orgs/go-gl/members hehe.

pwaller commented 10 years ago

LGTM.