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

Jump to bottom

x/mobile/gl: ctx.BufferData and ctx.BufferSubData with unsafe.Pointer #29720

Open
maja42 opened this issue Jan 13, 2019 · 8 comments
Open

x/mobile/gl: ctx.BufferData and ctx.BufferSubData with unsafe.Pointer #29720

maja42 opened this issue Jan 13, 2019 · 8 comments
Labels
mobile Android, iOS, and x/mobile Proposal Proposal-Accepted
Milestone
Unreleased

Comments

@maja42
Copy link

maja42 commented Jan 13, 2019

What version of Go are you using (go version)?

$ go version
go version go1.11.4 windows/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

When working with OpenGL, meshes consist out of vertices and indices that need to be pushed into GPU buffers.

The vertex array is usually of type []float32, the index array is usually of type []uint16 or []uint32.

The Context-object in golang.org/x/mobile/gl (gl.go and gldebug.go) offers the following methods for pushing such buffer data:

func (ctx *context) BufferData(target Enum, src []byte, usage Enum)
func (ctx *context) BufferSubData(target Enum, offset int, data []byte)

The problem here is that both functions require byte slices. In order to use them, the vertex/index arrays need to be converted first. If done in a safe way (without relying on go implementation details), the data effectively needs to be copied every time before it can be rendered.

When rendering large amounts of data this is a significant (and unnecessary) performance impact.

The implementation of Buffer(Sub)Data converts the incoming byte slice into an unsafe.Pointer - which is the format needed by the underlying OpenGL device. So this whole conversion is only needed to satisfy the function arguments.

Using only []byte within the application is rarely an option because vertex/index data needs to be manipulated to create dynamic scenes. This would only move the conversion to a different location.

What did you expect to see?

I suggest adding more generic functions for setting buffer data:

func (ctx *context) BufferData(target Enum, src unsafe.Pointer, size int, usage Enum)
func (ctx *context) BufferSubData(target Enum, offset int, data unsafe.Pointer, size int)

This would allow users to write any type of slice into buffers without expensive conversion or unsafe/fragile casting.

Alternatively (if the unsafe package should not face to the user) I suggest adding the following functions:

func (ctx *context) BufferData(target Enum, src []float32, usage Enum)
func (ctx *context) BufferData(target Enum, src []uint16, usage Enum)
func (ctx *context) BufferSubData(target Enum, offset int, data []float32)
func (ctx *context) BufferSubData(target Enum, offset int, data []uint16)
@gopherbot gopherbot added this to the Proposal milestone Jan 13, 2019
@maja42
Copy link
Author

maja42 commented Jan 13, 2019

For comparison, this is the slow and safe way for converting float32 arrays:

func float32asBytesSafe(data []float32) []byte {
	b := make([]byte, 4*len(data))
	for i, v := range data {
		u := math.Float32bits(v)
		b[4*i+0] = byte(u >> 0)
		b[4*i+1] = byte(u >> 8)
		b[4*i+2] = byte(u >> 16)
		b[4*i+3] = byte(u >> 24)
	}
	return b
}

And this is the fast, but unsafe and fragile way:

func float32asBytes(data []float32) []byte {
	if len(data) == 0 {
		return []byte{}
	}
	bsize := 4 * len(data)
	fptr := unsafe.Pointer(&(data[0])) // pointer to first byte of data
	barr := (*[1]byte)(fptr)           // interpret as pointer to byte-array
	bslice := (*barr)[:]               // convert to byte slice with len == cap == 1

	overwriteSliceBounds(&bslice, bsize)
	return bslice
}
func overwriteSliceBounds(s *[]byte, bounds int) {
	addr := unsafe.Pointer(s) // pointer to slice structure

	// Find addresses of len and cap fields (fragile! depends on go-internal structures)
	lenAddr := uintptr(addr) + uintptr(8)
	capAddr := uintptr(addr) + uintptr(16)
	// Create pointers to the length and cap fields
	lenPtr := (*int)(unsafe.Pointer(lenAddr))
	capPtr := (*int)(unsafe.Pointer(capAddr))

	// the next changes might corrupt data of the original data type (?)
	*lenPtr = bounds
	*capPtr = bounds
}

@beoran
Copy link

beoran commented Jan 15, 2019

I would say functions should be added that take []float32 for the vertices and, []uint16 for the indexes. Like that no unsafe pointer use will be needed.

@rsc rsc changed the title Proposal: x/mobile/gl: ctx.BufferData and ctx.BufferSubData with unsafe.Pointer proposal: x/mobile/gl: ctx.BufferData and ctx.BufferSubData with unsafe.Pointer Jan 16, 2019
@rsc
Copy link
Contributor

rsc commented Jan 16, 2019

/cc @nigeltao

@nigeltao
Copy link
Contributor

Yeah, we probably want new methods, with names such as BufferDataFloat32 since with Go, unlike C++, we can't overload methods.

/cc @crawshaw

@rsc
Copy link
Contributor

rsc commented Jan 23, 2019

@nigeltao are you comfortable saying we should accept this proposal?

@nigeltao
Copy link
Contributor

I'm comfortable saying we should accept this proposal, except the parts about adding unsafe.Pointer to the API.

@rsc
Copy link
Contributor

rsc commented Jan 30, 2019

Based on discussion, marking approved for adding more BufferData/BufferSubData with different type arguments, but not unsafe.Pointer. Note also that Go does not have method overloading, so you'd need to do:

func (ctx *context) BufferDataFloat32(target Enum, src []float32, usage Enum)
func (ctx *context) BufferDataUint16(target Enum, src []uint16, usage Enum)
func (ctx *context) BufferSubDataFloat32(target Enum, offset int, data []float32)
func (ctx *context) BufferSubDataUint16(target Enum, offset int, data []uint16)
etc.

@rsc rsc modified the milestones: Proposal, Unreleased Jan 30, 2019
@rsc rsc changed the title proposal: x/mobile/gl: ctx.BufferData and ctx.BufferSubData with unsafe.Pointer x/mobile/gl: ctx.BufferData and ctx.BufferSubData with unsafe.Pointer Jan 30, 2019
@gopherbot gopherbot added the mobile Android, iOS, and x/mobile label Jan 30, 2019
@gopherbot
Copy link

Change https://golang.org/cl/247077 mentions this issue: cl: add type specific BufferData methods to context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
mobile Android, iOS, and x/mobile Proposal Proposal-Accepted
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@beoran @rsc @maja42 @nigeltao @gopherbot