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

[bug] float * pointers #154

Open
DanielMazurkiewicz opened this issue Jun 2, 2022 · 15 comments
Open

[bug] float * pointers #154

DanielMazurkiewicz opened this issue Jun 2, 2022 · 15 comments

Comments

@DanielMazurkiewicz
Copy link

I guess it should look more like:

  arg = arg.replace('const ', '')
  if (arg.includes('*')) {
    if (arg.trim().startsWith("float")) {
      return 'number[] | Float32Array'
    }
    return 'number'
  }

and I guess you'll have to handle it also on napi level correctly

@RobLoach
Copy link
Owner

RobLoach commented Jun 2, 2022

Which method uses the float*?

@konsumer
Copy link
Collaborator

konsumer commented Jun 2, 2022

I'm not following. What is the issue?

@DanielMazurkiewicz
Copy link
Author

DanielMazurkiewicz commented Jun 2, 2022

@RobLoach

From this file:
https://raw.githubusercontent.com/raysan5/raylib/2e3cfdcc2f5c70e82536caa57d4aa72e3f00fd40/parser/raylib_api.json

This one:

    {
      "name": "LoadWaveSamples",
      "description": "Load samples data from wave as a floats array",
      "returnType": "float *",
      "params": [
        {
          "type": "Wave",
          "name": "wave"
        }
      ]
    }

see here:

export function LoadWaveSamples(wave: Wave): number

and

this one:

    {
      "name": "Mesh",
      "description": "Mesh, vertex data and vao/vbo",
      "fields": [
        {
          "type": "int",
          "name": "vertexCount",
          "description": "Number of vertices stored in arrays"
        },
        {
          "type": "int",
          "name": "triangleCount",
          "description": "Number of triangles stored (indexed or not)"
        },
        {
          "type": "float *",
          "name": "vertices",
          "description": "Vertex position (XYZ - 3 components per vertex) (shader-location = 0)"
        },
        {
          "type": "float *",
          "name": "texcoords",
          "description": "Vertex texture coordinates (UV - 2 components per vertex) (shader-location = 1)"
        },
        {
          "type": "float *",
          "name": "texcoords2",
          "description": "Vertex second texture coordinates (useful for lightmaps) (shader-location = 5)"
        },
        {
          "type": "float *",
          "name": "normals",
          "description": "Vertex normals (XYZ - 3 components per vertex) (shader-location = 2)"
        },
        {
          "type": "float *",
          "name": "tangents",
          "description": "Vertex tangents (XYZW - 4 components per vertex) (shader-location = 4)"
        },
        {
          "type": "unsigned char *",
          "name": "colors",
          "description": "Vertex colors (RGBA - 4 components per vertex) (shader-location = 3)"
        },
        {
          "type": "unsigned short *",
          "name": "indices",
          "description": "Vertex indices (in case vertex data comes indexed)"
        },
        {
          "type": "float *",
          "name": "animVertices",
          "description": "Animated vertex positions (after bones transformations)"
        },
        {
          "type": "float *",
          "name": "animNormals",
          "description": "Animated normals (after bones transformations)"
        },
        {
          "type": "unsigned char *",
          "name": "boneIds",
          "description": "Vertex bone ids, max 255 bone ids, up to 4 bones influence by vertex (skinning)"
        },
        {
          "type": "float *",
          "name": "boneWeights",
          "description": "Vertex bone weight, up to 4 bones influence by vertex (skinning)"
        },
        {
          "type": "unsigned int",
          "name": "vaoId",
          "description": "OpenGL Vertex Array Object id"
        },
        {
          "type": "unsigned int *",
          "name": "vboId",
          "description": "OpenGL Vertex Buffer Objects id (default vertex data)"
        }
      ]

see here:

export interface Mesh {

@DanielMazurkiewicz
Copy link
Author

@konsumer

I'm not following. What is the issue?

the issue is with types mappings: float * doesn't represent a single number, but entire array of numbers, above gave examples for the issue, hope self explanatory, but let me know if you need more explanation

@RobLoach
Copy link
Owner

RobLoach commented Jun 2, 2022

Thanks! Was having a hack to at getting the float[] to work too in the Parser update PR too.

Do you know of there's an example that uses one of these functions so we could test it further?

@DanielMazurkiewicz
Copy link
Author

Thanks! Was having a hack to at getting the float[] to work too in the Parser update PR too.

will have a look at your PR

Do you know of there's an example that uses one of these functions so we could test it further?

Nope, I don't have. And technically you have to decide first how that api and models will look like, to give you an example, this:

        {
          "type": "int",
          "name": "vertexCount",
          "description": "Number of vertices stored in arrays"
        },
        {
          "type": "float *",
          "name": "vertices",
          "description": "Vertex position (XYZ - 3 components per vertex) (shader-location = 0)"
        },

could be supplied as just vertices w/o count property as you can obtain array size from array object or typed array...

The simplest test would be to generate Mesh, get it in JS, and then draw it.
GenMeshXXX
DrawMesh

@DanielMazurkiewicz
Copy link
Author

One more thing I've noticed here and there:

    {
      "name": "LoadFileData",
      "description": "Load file data as byte array (read)",
      "returnType": "unsigned char *",
      "params": [
        {
          "type": "const char *",
          "name": "fileName"
        },
        {
          "type": "unsigned int *",
          "name": "bytesRead"
        }
      ]
    }

Notice, that bytesRead is actually a pointer to where to store single data (not array)

@twuky
Copy link
Collaborator

twuky commented Jun 10, 2022

@DanielMazurkiewicz I'm sorry I didn't see a notification for this one, but hopefully i can clear up some things about how the bindings are written here, and how we deal with pointers / lists. There's about a dozen functions that I know of in the library that aren't fully usable due to problems with pointers.

When, for example, the raylib function is meant to use a float* but the JS code expects to just use a number - it's not exactly anyone having forgotten to make it accept an array. Rather, in this case its saying that the user should pass in a JS number with the pointer address of a float array (in the program's C memory). At the current moment there's no real way for a user (from JS) to create a float array, or get the pointer address to it - so quite a few of these functions that require pointers aren't currently usable in node-raylib.

When handling structs that have arrays within them - like a Mesh - the type of anything with a * is meant to be converted to a JS number - its the pointer address of that list. The way this whole library works (at least right now) is that all data sent to/from the JS code and the NAPI bindings is copied over - so if we send back a Mesh that has a JS array, and the user modifies it, there could be some unexpected behavior anywhere pointers are involved - since the user isn't modifying the array at the original pointer location. So at the moment, these structs just send single numbers of the pointer addresses and there's no way to inspect or edit their internal data, unless there's a raylib function that can do it for you

Before committing any work to this repo I was working on a binding personally, which is sort of what the current bindings for node-raylib are now based on, and tried to list off the functions I came across that aren't usable from JS due to working with pointers: https://github.com/twuky/raylib-4.0/blob/master/unsupported_functions.md

I think the problem with getting a lot of these to work is that there isn't specifically one way to handle how a pointer is used by a function, which may mean the binding / NAPI code has to be very different for different functions - and the API definitions provided by raylib are ambiguous in that regard. So while we work from a code generator, I think a lot of the functions from the list linked above could be kind of edge cases, so the approach could be pretty difficult. For example, there's no way to tell if ImageResize() expects a pointer to a single Image or a pointer to the start of a list, without someone manually documenting it. So far the raylib API json file has been really helpful as keeping a single source of truth, but it falls short here.

I think it's worth going over the raylib API again to see if my list missed any functions that have issues with pointers, and create a larger issue/PR for this idea.

@twuky
Copy link
Collaborator

twuky commented Jun 10, 2022

I guess the short of what I'm saying above is that currently:

  • Almost all instances where a pointer/list are involved it is represented as a JS number because its meant to use or return the pointer address to something.
  • Which does make some functions that expect pointer arrays currently broken. Check the link above for the list of functions I've found that aren't currently really usable from JS.
  • For some things, like structs that contain arrays, this all may be necessary to keep permanent references to the original data inside the structs (and therefore users from JS can't inspect or modify lists inside of structs)

@konsumer
Copy link
Collaborator

konsumer commented Jun 11, 2022

I am still thinking we could use ref-types in js-space here, like ref-array-di.

Basically, you can hold a reference if you keep access to the buffer that backs the array, in the wrapper.

import ref from 'ref-napi'
import ar from 'ref-array-di'
import example from './build/Release/example'

const ArrayType = ar(ref)
const ArrayFloat = ArrayType(ref.types.float)

// this is an example js-space wrapper with an ArrayFloat input
function exampleInput (aInput) {
  const ia = aInput.instanceOf(ArrayFloat) ? aInput : new ArrayFloat(aInput)
  return example.exampleInput(ia.ref().address())
}

// make a new one
// you can also use an int for length, for empty array
const a = new ArrayFloat([0, 0, 0, 0])

// just some nice floats
a[0] = Math.PI
a[1] = Math.LN2
a[2] = Math.E
a[3] = Date.now() / 1000

// gets you the underlying buffer-store, if you need that
console.log(a.ref())

// gets you the actual memory address
console.log(a.ref().address())

// this will pass the pointer
console.log(exampleInput(a))

// this will less efficiently make a new ArrayFloat and pass the pointer
console.log(exampleInput([1, 2, 3, 4]))

I only did exampleInput, but in the ref-napi README, they mention "Read/write Buffer instances' memory addresses to other Buffer instances", so I think similar could be done on return (like NAPI returns a ref, take that ref and make a new buffer, give it the ref address.)

I need to play around with it a bit, but I think this might be a solution. They also have similar stuff for buffer-backed structs with pointers. I feel pretty strong on the JS-side of this, but I am not clear on the C-side at all.

I will setup a demo repo and try to figure it out, but it might be helpful to work on it with @twuky or @RobLoach, since ya'll are better with C.

@DanielMazurkiewicz
Copy link
Author

Thanks @konsumer and @twuky for the explanations, example and answers!

Almost all instances where a pointer/list are involved it is represented as a JS number because its meant to use or return the pointer address to something.

I don't think that squeezing pointer to double type is a good pattern, not mentioning using pointers in general in nodejs. There are typed arrays and these can be passed through NAPI. And on NAPI level you can even determine if it is a typed array or regular array and still supply a C code with proper data that doesn't require hacks on nodejs side. I see your point of single source of truth with that json file, but that can be still easily patched with "exceptions" json file where necessary.

This is not complaining, it is just my opinion. Still have a respect for you guys that you did it and taking care of it! :-)

@konsumer
Copy link
Collaborator

konsumer commented Jun 15, 2022

Our current setup is mostly generated, and personally, I am very weak with C, so any assistance would be appreciated. I can help with a PR, if you need a hand on the js/generation side, if you are interested. I started experimenting with those sort of ref types, but I can't work out what it needs to do on the C side, if you are interested in taking a look. It has everything all setup, basically just needs to pass the sort of refs we want to pass back & forth.

And on NAPI level you can even determine if it is a typed array or regular array and still supply a C code with proper data that doesn't require hacks on nodejs side.

I'd love to see a simple example. Can you make one?

I see your point of single source of truth with that json file, but that can be still easily patched with "exceptions" json file where necessary.

Yes, we do this quite a bit.

@DanielMazurkiewicz
Copy link
Author

Don't have a time to dig it now but that would be a starting point:
https://github.com/nodejs/node-addon-api/blob/main/doc/typed_array_of.md#data

and one of my old "napi" repos could be helpful too:
https://github.com/DanielMazurkiewicz/ultradb/blob/master/src/generic/napiMacros.h

If you won't solve it by the weekend I'll dig it and provide you with some example

@twuky
Copy link
Collaborator

twuky commented Jun 15, 2022

Being able to work directly with arrays would be a benefit in some places, but i don't think it solves all the problems we need to as far as pointers are concerned - but maybe this should be a topic for its own issue.

Are these libraries able to handle arrays of structs very easily? Some examples of where this may be tricky are LoadFontData(), DrawMeshInstanced(), or even DrawTexturePoly() (though i suspect we could work around arrays of vectors easily).

Then there are some functions where pointer types aren't pointing to lists that we may still have to deal with in some way. Would it be better to work with BigInts rather than doubles here? I think it should still work on the versions node-raylib supports, so it may be simple to swap the code generators to use them for the time being.

@konsumer
Copy link
Collaborator

konsumer commented Jun 15, 2022

Are these libraries able to handle arrays of structs very easily?

As far as I understand it, yes. They operate on a chunk of memory, and you can pass a pointer to other things to also operate on the chunk, so as long as the 2 sides agree how to work with the chunk, it's fine. In he case of structs and arrays, they agree, so it all works together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants