Closed pineapplemachine closed 5 years ago
it is worth saying that although this method is good at the initial stage, it does not give the desired severity of the types.
-> @return {Object.<string, WebGLBuffer>} The created sphere buffers
<string,
here you need to specify the properties explicitly
@return {{
position: WebGLBuffer,
normal: WebGLBuffer,
texcoord: WebGLBuffer,
indices: WebGLBuffer,
}} The created sphere buffers
for
Example:
*
* const bufferInfo1 = twgl.createBufferInfoFromArrays(gl, {
* position: [1, 2, 3, ... ],
* });
* const bufferInfo2 = twgl.createBufferInfoFromArrays(gl, {
* position: bufferInfo1.attribs.position, // use the same buffer from bufferInfo1
* });
This case should be taken into account in the typing, which I showed in my example I'm not a big expert on how to do this through js-doc, but in ts..
I think a good solution would be to generate files for each file in src
and correct them, and then make into Assembly by the ts compiler. Although this will require a lot of effort, it will ease the work with d.ts in the future.
@NikitaIT The example you gave would be better solved by improving the documentation rather than manually editing the definitions. See TextureOptions for an example of how a SphereBufferOptions or similar type could be represented in the docs.
However, I do not think that this is a blocking issue for this PR.
On Sat, Jan 12, 2019, 7:01 PM Никита notifications@github.com wrote:
I think a good solution would be to generate files for each file in src and correct them, and then make into Assembly by the ts compiler. Although this will require a lot of effort, it will ease the work with d.ts in the future.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/greggman/twgl.js/pull/98#issuecomment-453763358, or mute the thread https://github.com/notifications/unsubscribe-auth/AG7gbE8f8A5XCFCDFBSGntbscdnATQ-Dks5vChTMgaJpZM4Z8H28 .
I incorporated this PR into https://github.com/greggman/twgl.js/pull/97
Your python translated to node/js is here
https://github.com/greggman/twgl.js/blob/2085445b9159cff6b9a6add9d323b6b6d7d03d85/Gruntfile.js#L356
I tested it builds. I didn't test that it runs or that I translated every regex correctly. To build
npm install
npm run build
To just build the type file
npm run buildts
<string,
here you need to specify the properties explicitly@return {{ position: WebGLBuffer, normal: WebGLBuffer, texcoord: WebGLBuffer, indices: WebGLBuffer, }} The created sphere buffers
this doesn't feel right to me.
To me they should be documented as returning a map of strings to WebGLBuffer like
Record<string, WebGLBuffer>
Closing this PR; please see https://github.com/greggman/twgl.js/pull/102 instead
Record<string, WebGLBuffer>
Why? this makes the code more complex in ts you always rely on the types, and this type can not be relied on, this at least contradicts the Occam's razor principle of good code
perhaps this description will look better, this is the option I showed in ts
Record<'position | 'normal' | 'indices' | 'texcoord', WebGLBuffer>
@NikitaIT has a point. The more specific typing is better TypeScript.
However, I'd maintain that this is outside the scope of just getting something that works into a release. It can be polished up to maximum type-y goodness afterward.
On Tue, Jan 15, 2019, 6:18 PM Никита notifications@github.com wrote:
Record<string, WebGLBuffer> Why? this makes the code more complex in ts you always rely on the types, and this type can not be relied on, this at least contradicts the Occam's razor principle of good code
— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/greggman/twgl.js/pull/98#issuecomment-454452237, or mute the thread https://github.com/notifications/unsubscribe-auth/AG7gbGwYvQOYgrkyPXx4_MuQRGqoU3_Vks5vDf88gaJpZM4Z8H28 .
@pineapplemachine I'm not saying it's worth doing right now) but it's definitely worth it
I'd like to understand the call for stronger types. Record<string, WebGLBuffer>
vs
@return {{
position: WebGLBuffer,
normal: WebGLBuffer,
texcoord: WebGLBuffer,
indices: WebGLBuffer,
}}
I have lots of C++/C# experience so I'm not allergic to types I don't really understand a hard coded type makes sense for attribute/buffer related things.
it's perfectly valid to do this
const buffers = createSphereBuffers(...);
buffers.colors = someWebGLBuffer;
Typing createSphereBuffers as returning some specific list doesn't make sense to me but maybe I don't understand the suggestion. In C++/C# making a hard coded type with 4 explicit proprties would mean adding a 5th property is prohibited. Is this different in typescript?
All the functions that deal with buffers, attributes, and uniforms are explicitly string based maps to objects. There is no special meaning . You can make a shader
attribute float foo;
attribute vec3 bar;
and then make arrays
const bufferInfo = twgl.createBufferInfoFromArrays(gl, {
foo: { numComponents: 1, data: [1,2,3,4] },
bar: { numComponents: 3, data[ 1, 2, 3, 4, 5, 6, 7, 8, 9, 11, 11, 12],
};
In general position
, texcoord
, normal
etc are not special
Similarly you're equally free to do something like
const sphereArrays = twgl.primitives.createSphereArrays(10,20,30);
const arrays = {
foo: { numComponents: 1, data: [1,2,3,4, ...] },
bar: sphereArrays.position,
};
or like above
const sphereArrays = twgl.primitives.createSphereArrays(10,20,30);
sphereArrays.colors = { numComponents: 3, data: [1,2,3,4, ...] };
I don't mind stronger types if typescript has some special way of saying "this thing is a map of strings to X and starts with these specific 4 things but youre free to add or remove more things to the map" but coming from C++/C# that can't be expressed there AFAIK so I'm assuming it also can't be expressed in typescript?
@greggman There are tools to work around that sort of situation in TypeScript, since it's just JS and dynamic typing under the hood. Here is one option that I think could work better than simply <string, WebGLBuffer>
and may address your concerns:
interface ObjectWithAtLeastTheseProperties {
a: string;
b: string;
c: string;
[key: string]: string;
}
function doStuff() {
const abc: ObjectWithAtLeastTheseProperties = {
a: "apple",
b: "bear",
c: "clue",
};
abc.d = "door";
}
Another option is leaving it up to the dependent code to specify if they want to add any more properties to the object or not. For example, you could strictly define the input type as was first discussed and then in cases where somebody wanted to mess with the object's properties, they'd write something more like this:
const buffers = <[key: string]: WebGLBuffer> createSphereBuffers(...);
buffers.colors = someWebGLBuffer;
Or for even more freedom, something like this:
const buffers = <Object> createSphereBuffers(...);
buffers.colors = someWebGLBuffer;
buffers.whatever = "Hello, world!"
1) { a?: T, b: T } & { a: T } === { a: T, b: T }
2) { [key: keyof { a?: T, b: T } ]: K} === { a?: K, b: K }
3) <S extends { a?: T, b: T }>{ [key: keyof S ]: K}
4) M = <S>{ [key: keyof (S & { a?: T, b: T }) ]: K}
5) H = <S>{ [key: keyof (S & { a?: T, b: T }) ]: W}
6) G = <S, L>{ [key: keyof (S & { a?: T, b: T }) ]: L}
7) fn<A>(a: G<A, []>) : G<A, WebglBuffer>
8) buffers = fn(array) // fn<A> auto derived
See code not see where in it user-defined type of https://github.com/greggman/twgl.js/blob/c69de9ae95755fc00939a8c9d65f8701aa2d79f0/src/primitives.js#L643
return {
position: positions,
normal: normals,
texcoord: texcoords,
indices: indices,
};
This is TRecord = Record<'position' | 'normal' | 'indices' | 'texcoord', WebGLBuffer>
.
But user-defined type of https://github.com/greggman/twgl.js/blob/c69de9ae95755fc00939a8c9d65f8701aa2d79f0/src/attributes.js#L701
createBuffersFromArrays<TRecord, TUserArrayNames>(gl, arrays: Record<keyof TRecord & TUserArrayNames, []>): TRecord & Record<TUserArrayNames, WebGLBuffer> {}
// TUserArrayNames is derived from the arrays
or TUserArrayNames = keyof TUserArrayByName
and user use like:
const arrays: { abs: [], position: [] } = ... ;
const buffers: Record<'position' | 'abs', WebGLBuffer> = createSphereBuffers(...); // and this type is derived from the arguments(arrays)
enum NameEnum {
position,
normal,
texcoord,
indices,
}
class Buffer<TNameEnum = NameEnum> whare TNameEnum: Enum {
...
public WebGLBuffer this[TNameEnum i]
{
get { return arrayByName[i]; }
}
}
and user can create self enum or extends(i know what enum not extendable, but exists many ways escape this)
This PR includes several documentation fixes that also fix TypeScript declaration issues.
I used this script to transform the grunt output into a syntactically valid TypeScript declaration file. I've tested to the extent that my project using a tiny slice of the API doesn't immediately die. Any further errors are probably mistakes in the documentation. For example, I only ran into
TextureOptions.color
incorrectly being marked as a mandatory attribute (when in fact it is optional) because I happen to be using TextureOptions in my small project that I used to help with testing, and I got a compilation error complaining that I hadn't included acolor
attribute.I'm not familiar with your build process @greggman so I'm gonna go ahead and request that you take this across the finish line, incorporating this script (...probably rewritten in JS. sorry, I'm faster at text processing in Python) as part of the TS declarations build process. You might also consider making some of these changes in the documentation directly, for example replacing "?" to document functions that accept an arbitrary type with "any"; and/or replacing "enum" with the perhaps more conventional "GLenum". (Right now, the script must change these things in the declarations file.)