ungerik / go3d

A performance oriented 2D/3D math package for Go
MIT License
303 stars 49 forks source link

Feature request: T.Array() #7

Closed dertseha closed 9 years ago

dertseha commented 9 years ago

The methods T.Slice() returns a slice to access the single fields, but this is incompatible with functions such as from go-gl on the UniformLocation type to specify some matrix values. Theses methods have arrays in their signature:

func (location UniformLocation) UniformMatrix4fv(transpose bool, list ...[16]float32) {

as seen here

There is already one copy operation per field going on, I'd like to avoid having a second operation to copy the slice to a compatible array. Hence I'd like to propose the feature to add T.Array(), such as mat4.Array() [16]float32.

Do you see this as a suitable addition to the library? Would you want to do this on your own or via pull request?

ungerik commented 9 years ago

Using an array is very descriptive for the function interface, but I guess passing a slice would be more efficient since it's only a reference and doesn't copy 64 bytes. I would rather change the go-gl interface.

I could implement mat4.T as float array, but then we would miss out on the handy feature of it being an array of 4 vectors.

So let's compromise on adding a mat4.T.Array() [16]float32 method. Send me a pull request for it.

-Erik

On Mon, Aug 25, 2014 at 10:29 PM, Christian Haas notifications@github.com wrote:

The methods T.Slice() returns a slice to access the single fields, but this is incompatible with functions such as from go-gl on the UniformLocation type to specify some matrix values. Theses methods have arrays in their signature:

func (location UniformLocation) UniformMatrix4fv(transpose bool, list ...[16]float32) {

as seen here https://github.com/go-gl/gl/blob/2cf494e17e96fdd93743d8c16637f1261aafda82/uniformlocation.go#L132

There is already one copy operation per field going on, I'd like to avoid having a second operation to copy the slice to a compatible array. Hence I'd like to propose the feature to add T.Array(), such as mat4.Array() [16]float32.

Do you see this as a suitable addition to the library? Would you want to do this on your own or via pull request?

— Reply to this email directly or view it on GitHub https://github.com/ungerik/go3d/issues/7.

+43 650 58 33055 erik@erikunger.com Skype: ungerik http://erikunger.com http://github.com/ungerik http://twitter.com/ungerik http://facebook.com/ungerik http://www.linkedin.com/in/ungerik http://www.xing.com/profile/Erik_Unger2

alexozer commented 9 years ago

You could also return a pointer, as in mat4.T.Array() *[16]float32 So the entire thing isn't passed back.

dertseha commented 9 years ago

Right, I was thinking about this. By then using functions such as UniformMatrix4f there would be just one copy operation.

Using an array internally as well would be the best optimization (no copy at all), like gl-matrix.js does it. But first an application must be written where this is a big performance issue ;)

alexozer commented 9 years ago

My pull req: https://github.com/ungerik/go3d/pull/8

ungerik commented 9 years ago

I have solved the problem by re-interpreting the bytes via the unsafe package.

-Erik

On Tue, Aug 26, 2014 at 10:18 PM, Alex Ozer notifications@github.com wrote:

You could also return a pointer, as in mat4.T.Array() *[16]float32 So the entire thing isn't passed back.

— Reply to this email directly or view it on GitHub https://github.com/ungerik/go3d/issues/7#issuecomment-53483269.

+43 650 58 33055 erik@erikunger.com Skype: ungerik http://erikunger.com http://github.com/ungerik http://twitter.com/ungerik http://facebook.com/ungerik http://www.linkedin.com/in/ungerik http://www.xing.com/profile/Erik_Unger2

dertseha commented 9 years ago

I finally had time to look at the update - Thank you for the extension! Sadly, it only partially works for me now. In my project, I'm doing environment-agnostic GL rendering, both for native and browser. For the browser, I'm using gopher-js, which transpiles the Go code to JavaScript. Before the change, I could use the Slice() method; Now it fails for the browser and Array() wouldn't work either, as this one creates the problem: The usage of the unsafe pointer is lost in the JS conversion and the JS code simply returns the wrapped array of 4 arrays (mat4 in this case). In the end, UniformMatrix complains about receiving an array with 4 elements (instead of 16).

So far I haven't thought about a possible solution, only various ideas. The worst case might be I'll have to copy the matrix cell-by-cell and then pass it on in a compatible format.

ungerik commented 9 years ago

On Tue, Sep 16, 2014 at 11:43 AM, Christian Haas notifications@github.com wrote:

The usage of the unsafe pointer is lost in the JS conversion and the JS code simply returns the wrapped array of 4 arrays (mat4 in this case). In the end, UniformMatrix complains about receiving an array with 4 elements (instead of 16).

We could use a build constraint to use a conventional copy function instead of unsafe in case of GopherJS.

// +build gopherjs

go build -tags 'gopherjs'

If GopherJS doesn't have such a tag, then we should request this as feature since it's clearly its own platform.

-Erik

+43 650 58 33055 erik@erikunger.com Skype: ungerik http://erikunger.com http://github.com/ungerik http://twitter.com/ungerik http://facebook.com/ungerik http://www.linkedin.com/in/ungerik http://www.xing.com/profile/Erik_Unger2

ungerik commented 9 years ago

OK, GopherJS uses the build tag "netgo".

-Erik

On Tue, Sep 16, 2014 at 12:16 PM, Erik Unger erik@erikunger.com wrote:

On Tue, Sep 16, 2014 at 11:43 AM, Christian Haas <notifications@github.com

wrote:

The usage of the unsafe pointer is lost in the JS conversion and the JS code simply returns the wrapped array of 4 arrays (mat4 in this case). In the end, UniformMatrix complains about receiving an array with 4 elements (instead of 16).

We could use a build constraint to use a conventional copy function instead of unsafe in case of GopherJS.

// +build gopherjs

go build -tags 'gopherjs'

If GopherJS doesn't have such a tag, then we should request this as feature since it's clearly its own platform.

-Erik

+43 650 58 33055 erik@erikunger.com Skype: ungerik http://erikunger.com http://github.com/ungerik http://twitter.com/ungerik http://facebook.com/ungerik http://www.linkedin.com/in/ungerik http://www.xing.com/profile/Erik_Unger2

+43 650 58 33055 erik@erikunger.com Skype: ungerik http://erikunger.com http://github.com/ungerik http://twitter.com/ungerik http://facebook.com/ungerik http://www.linkedin.com/in/ungerik http://www.xing.com/profile/Erik_Unger2

ungerik commented 9 years ago

Done.

dertseha commented 9 years ago

Thank you! This works; I didn't know about build flags.