search

go-gl / mathgl

A pure Go 3D math library.
BSD 3-Clause "New" or "Revised" License
554 stars 65 forks source link

Math32 #52

Closed ghost closed 6 years ago

ghost commented 8 years ago

This PR changes mgl32 to use a float32 version of standard library math

UserAB1236872 commented 8 years ago

Hmm, there are merge conflicts I'll have to look at, but as long as you can confirm this passes tests this LGTM.

ghost commented 8 years ago 
10:mgl32 hydroflame$ go test 
PASS
ok      github.com/hydroflame/mathgl/mgl32  0.010s
10:mgl32 hydroflame$ cd ../mgl64
10:mgl64 hydroflame$ go test
PASS
ok      github.com/hydroflame/mathgl/mgl64  0.010s
ghost commented 8 years ago

I tried to check for merge conflicts by merging master on my branch and then pushing and nothing came up... so I'm not sure what github is complaining about

dmitshur commented 8 years ago

This branch is 8 commits ahead, 11 commits behind go-gl:master.

You should rebase your math32 branch on top of latest master of mathgl.

ghost commented 8 years ago

Oh i see, I don't have access to write to go-gl/mathgl, how do i pull from a fork ?

dmitshur commented 8 years ago

You only need read access to pull, and then be able to rebase.

  1. In your fork, add the go-gl upstream as a remote:

    $ git remote add upstream https://github.com/go-gl/mathgl

  2. Fetch from the remote:

    $ git fetch upstream

  3. That should populate upstream/master branch with the latest commit. Now, you can rebase your branch:

    $ git checkout math32 # If it's not already checked out.
$ git rebase upstream/master

Then force push your updated math32 branch to your fork.

I believe that should work.

ghost commented 8 years ago

Can someone review ?

Actually hold on Ill squash everythign so it's easilly revertable

slimsag commented 8 years ago

Looks like it's rebased and all is well, so LGTM. I haven't reviewed the actual changes, though (assuming @Jragonmiris LGTM still stands).

dmitshur commented 8 years ago

I think reviewing actual changes is important, given how low level this library is.

I'm concerned about adding a dependency to another package from a different organization that as far as I know none of us except @hydroflame have access to. It'd be the first non-standard library and non-external standard library dependency for mathgl.

Can you justify the need to do that @hydroflame? I'm not against it, I'd just like to have some points that explain why doing this is better than not doing it.

Also, 3 out of 4 badges on https://github.com/luxengine/math#readme are not loading, and clicking on the tests badge prints "Not Found".

image

ghost commented 8 years ago

My CI machine broke and I haven't re start it

We should replace all these function call purely for performance reason, I squeeze a few extra ns out of my native float32 calls

I can vendor the library if you'd feel better with that

UserAB1236872 commented 8 years ago

Vendoring would probably be appropriate. I would like to see the badges fixed just so we can see the CI status before any merge.

ghost commented 8 years ago

That won't be anytime soon as I don't have any server anymore, but it's basically all test pass and coverage is 100% The entire package is a copy of standard library math. I just replaced pieces one by one for native implementations

dmitshur commented 8 years ago

The entire package is a copy of standard library math. I just replaced pieces one by one for native implementations

Is it generated or modified by hand? Based on https://github.com/luxengine/math/commits/master, it appears to be the latter, but can you confirm?

ghost commented 8 years ago

It is modified by hand, I started by making a forwarding function call for everything 

func Abs(f float32) float32 {
    return float32(math.Abs(float64(f)))
}

then manually replaced these with the float32 or ASM implementation. unfortunatelly its not as simple as search and replace

ghost commented 8 years ago

The badges are avail now.

UserAB1236872 commented 8 years ago

LGTM

@shurcooL ?

ghost commented 8 years ago

Wait but do we want vendor or not ?

dmitshur commented 8 years ago

Wait but do we want vendor or not ?

Yeah, let's figure that out.

I'll try to get this reviewed this weekend. Added it to my backlog.

This PR has some merge conflicts now (probably minor).

dmitshur commented 8 years ago

I've reviewed this more carefully now. This is hard to say (as in, it would've been easier for me to just say "LGTM"), but I don't think this change should be merged as is. Here's my rationale.

I look at it from a benefit and cost analysis. Right now, this change comes with a 1 line description:

This PR changes mgl32 to use a float32 version of standard library math

Given the scope of the change, I don't think that is enough to justify why it should be done. I'd really like to be able to make decisions based on numbers, so I think benchmarks should be provided as part of this PR in order to explain why this should or should not be merged. I look at the Go project and changes of similar nature are always accompanied by detailed explanation.

Without benchmarks, I can't know what the effect of this PR is. Is the performance improved by 1%? By 250%? Or is performance actually made worse?

On the costs side, I see the following costs:

Previously, if there's an issue in this library, a maintainer must do these steps:

  1. Verify if the issue is real.
  2. Make a PR to mathgl and fix it.
  3. Wait for a go-gl maintainer to +1 it and merge.

After this change, a maintainer would have to:

  1. Verify if the issue is real.

  2. Figure out if the issue is inside go-gl/mathgl or in luxengine/math.

  3. If in mathgl, make a PR to mathgl and fix it.

  4. Wait for a go-gl maintainer to +1 it and merge.

  5. If in luxengine/math, make a PR to luxengine/math and fix it.

  6. Wait for a luxengine/math maintainer (only @hydroflame?) to +1 it and merge.

  7. If we're using vendoring, update the vendored copy in mathgl.

My main concern is that there are few maintainers in go-gl, and there are few ones who are very active and very responsive. It's not uncommon for valid issues to be reported in these libraries, and they go unaddressed for days, weeks, sometimes months. We have many open and unresolved issues. I'd like to help improve that situation, not make it worse.

With that, the benefits (?) don't seem to outweigh the costs to me.

I want this mathgl library to be as good as possible overall, and it seems merging this PR (as is) would not make it better.

If mathgl were to be contributed upstream into the Go project into a place similar to https://godoc.org/golang.org/x/image/math, would this change make it more likely to be accepted or less? It seems to be that it would be less.

I have the following questions/requests to @hydroflame to make this change easier to accept:

I think @Jragonmiris should make the final decision on what he thinks is best direction for this library, I'm okay with deferring the final decision to him. The above is just my opinion based on my knowledge.

Conclusion

Let me make it very clear that I am not against making this change, but I do not think this PR should be merged outright as is unless the following concerns are resolved:

I'm also surprised this PR does not say that it resolves issue #28. I think that's an important point to mention.

ghost commented 8 years ago

I'm almost certain that my math library is as correct as the standard library math library, I reused the same tests and in fact found 2 bugs in stdlib (that are now fixed). The speedup is noticeable, Sqrt is twice as fast, but not that important since no one complained so far. As far as the maintaining problem. I wholeheartedly agree with you on that. I'm not the principal author of mathgl and I don't work on it a lot because I made my own github.com/luxengine/glm so I will not take a stance vis-a-vis that particular issue, you guys decide what you want to do.

I didn't notice issue #28 but yeah it does.

dmitshur commented 8 years ago

This PR seems invalid now because https://github.com/luxengine/math is 404. I've tried asking @hydroflame about what happened but never got a response. So I'm not sure what's going on or what happened. 😕

dmitshur commented 6 years ago

I'll close this PR because it's out of date and the repository from which the PR was made has been deleted.

Issue #28 can still be taken on, but the concerns raised in https://github.com/go-gl/mathgl/pull/52#issuecomment-214064629 should be addressed by any PR that tries to do so.