(Start of) TypeScript port and code splitting #2
Conversation
completely untested and 0 functionality changes, just ports the JavaScript to TypeScript 1:1 as much as possible. the only changes made are to get the TypeScript compiler to shut up
|
Thanks for your effort! Anything to clean this up helps.
Nice. As for the forced JSDoc in the eslint, I realize it's pretty annoying. I like it from an AuDHD cleanfreak perspective, but definitely not necessary with TS to put it everywhere. I realize not using TS just to have this much JSDoc is pretty counterintuitive. It's mostly me not wanting to leave my comfort zone.
Is the factory stuff you're mentioning the UMD? It's in struct-fu too, and those definitely have to go. This is once again my fault from being pesky and not wanting to dive into the SoyScript Ecosystem, but the workarounds for these are ugly. They can still be converted to UMD in the final build, right.
The eslint config here is actually copy-pasted from yours, but with changes including forced JSDoc.
I was thinking of removing that outright, less "state" to worry about.
I've been thinking about doing this too, everything you're mentioning I hate as well and are things TypeScript can solve. I'm also thinking about how the function to convert from Mii Studio data seems sort of redundant 😔 (especially the "codec utilities", out of place for this lib. Who wrote this cr*p? 😂) |
I was the exact same way for years, but after making the jump to TypeScript I won't be going back. Tools like tsup also make the build step part trivial in most cases, so no worries there 👍
Yeah it's the UMD. My main goal for this PR was to change as little as possible in terms of functionality, just porting to TypeScript in a way that still let things build and be usable. The UMD implementation here seems to want to fight back on me with that, so it might warrant some actual functionality changes
They can yeah. TypeScript supports
Ah okay, sounds good then. Yes ours can be easily imported, we broke our eslint rules out into their own package a while ago so they can be easily shared between projects, which makes it trivial to import the rules anywhere else too https://npmjs.com/@pretendonetwork/eslint-config
Sounds good 👍 That might be out of scope here now that I think about it more, but that's good to know for the future
Awesome, I went ahead and pulled it out into it's own library here https://github.com/PretendoNetwork/binary-parser. I believe I covered all the bases needed here so far? Let me know if not, I'd be happy to see about making additions. The library definitely isn't done yet (it's not even minified or anything atm lol) You mentioned using |
Nice, glad you released this on its own. I'm afraid to say it's too late as I finished making manual decoders for the lib. I'll definitely have a look at some point though, like, I wish I had more options before choosing struct-fu 🙂 I've been spending the past week addressing low hanging fruit in the library: using all ESM, removing struct-fu, and having no more console.debug calls. To circle back then, well I wanted to ask what your goals are here. Or maybe you just began this from impulse and didn't expect anything out of it. |
I see. If bringing in the other parser is not something desired anymore, I won't push for it. Though I will say that there are still some benefits to doing so. One of the corner stones of our parser is tight integration with the TypeScript type system. That means type generation is a 1st class feature. Using our library means functions like: /**
* @typedef {Object} FFLiCharInfo
* @property {number} miiVersion
* @property {number} faceType
* @property {number} faceColor
* @property {number} faceTex
* @property {number} faceMake
* @property {number} hairType
* @property {number} hairColor
* @property {number} hairFlip
* @property {number} eyeType
* @property {number} eyeColor
* @property {number} eyeScale
* @property {number} eyeAspect
* @property {number} eyeRotate
* @property {number} eyeX
* @property {number} eyeY
* @property {number} eyebrowType
* @property {number} eyebrowColor
* @property {number} eyebrowScale
* @property {number} eyebrowAspect
* @property {number} eyebrowRotate
* @property {number} eyebrowX
* @property {number} eyebrowY
* @property {number} noseType
* @property {number} noseScale
* @property {number} noseY
* @property {number} mouthType
* @property {number} mouthColor
* @property {number} mouthScale
* @property {number} mouthAspect
* @property {number} mouthY
* @property {number} beardMustache
* @property {number} beardType
* @property {number} beardColor
* @property {number} beardScale
* @property {number} beardY
* @property {number} glassType
* @property {number} glassColor
* @property {number} glassScale
* @property {number} glassY
* @property {number} moleType
* @property {number} moleScale
* @property {number} moleX
* @property {number} moleY
* @property {number} height
* @property {number} build
* @property {string} name
* @property {string} creator
* @property {number} gender
* @property {number} birthMonth
* @property {number} birthDay
* @property {number} favoriteColor
* @property {number} favorite
* @property {number} copyable
* @property {number} ngWord
* @property {number} localonly
* @property {number} regionMove
* @property {number} fontRegion
* @property {number} roomIndex
* @property {number} positionInRoom
* @property {number} birthPlatform
* @property {Uint8Array} createID
* @property {number} padding_0
* @property {number} authorType
* @property {Uint8Array} authorID
*/
const FFLiCharInfo_size = 288;
/**
* @param {Uint8Array} u8 - module.HEAPU8
* @param {number} ptr - Pointer to the type.
* @returns {FFLiCharInfo} Object form of FFLiCharInfo.
* @package
*/
function _unpackFFLiCharInfo(u8, ptr) {
const view = new DataView(u8.buffer, ptr);
const name = new TextDecoder('utf-16le').decode(new Uint16Array(u8.buffer, ptr + 180, 11));
const creator = new TextDecoder('utf-16le').decode(new Uint16Array(u8.buffer, ptr + 202, 11));
const createID = new Uint8Array(u8.buffer, 264, 10);
const authorID = new Uint8Array(u8.buffer, 280, 8);
return {
miiVersion: view.getInt32(0, true),
faceType: view.getInt32(4, true),
faceColor: view.getInt32(8, true),
faceTex: view.getInt32(12, true),
faceMake: view.getInt32(16, true),
hairType: view.getInt32(20, true),
hairColor: view.getInt32(24, true),
hairFlip: view.getInt32(28, true),
eyeType: view.getInt32(32, true),
eyeColor: view.getInt32(36, true),
eyeScale: view.getInt32(40, true),
eyeAspect: view.getInt32(44, true),
eyeRotate: view.getInt32(48, true),
eyeX: view.getInt32(52, true),
eyeY: view.getInt32(56, true),
eyebrowType: view.getInt32(60, true),
eyebrowColor: view.getInt32(64, true),
eyebrowScale: view.getInt32(68, true),
eyebrowAspect: view.getInt32(72, true),
eyebrowRotate: view.getInt32(76, true),
eyebrowX: view.getInt32(80, true),
eyebrowY: view.getInt32(84, true),
noseType: view.getInt32(88, true),
noseScale: view.getInt32(92, true),
noseY: view.getInt32(96, true),
mouthType: view.getInt32(100, true),
mouthColor: view.getInt32(104, true),
mouthScale: view.getInt32(108, true),
mouthAspect: view.getInt32(112, true),
mouthY: view.getInt32(116, true),
beardMustache: view.getInt32(120, true),
beardType: view.getInt32(124, true),
beardColor: view.getInt32(128, true),
beardScale: view.getInt32(132, true),
beardY: view.getInt32(136, true),
glassType: view.getInt32(140, true),
glassColor: view.getInt32(144, true),
glassScale: view.getInt32(148, true),
glassY: view.getInt32(152, true),
moleType: view.getInt32(156, true),
moleScale: view.getInt32(160, true),
moleX: view.getInt32(164, true),
moleY: view.getInt32(168, true),
height: view.getInt32(172, true),
build: view.getInt32(176, true),
name,
creator,
gender: view.getInt32(224, true),
birthMonth: view.getInt32(228, true),
birthDay: view.getInt32(232, true),
favoriteColor: view.getInt32(236, true),
favorite: view.getUint8(240),
copyable: view.getUint8(241),
ngWord: view.getUint8(242),
localonly: view.getUint8(243),
regionMove: view.getInt32(244, true),
fontRegion: view.getInt32(248, true),
roomIndex: view.getInt32(252, true),
positionInRoom: view.getInt32(256, true),
birthPlatform: view.getInt32(260, true),
createID,
padding_0: view.getUint16(274, true),
authorType: view.getInt32(276, true),
authorID
};
}Can be reduced down to something like: import * as b from '@pretendonetwork/binary-parser';
const ffliCharInfoParser = new b.Parser()
.endianness('little')
.int32('miiVersion')
.int32('faceType')
.int32('faceColor')
.int32('faceTex')
.int32('faceMake')
.int32('hairType')
.int32('hairColor')
.int32('hairFlip')
.int32('eyeType')
.int32('eyeColor')
.int32('eyeScale')
.int32('eyeAspect')
.int32('eyeRotate')
.int32('eyeX')
.int32('eyeY')
.int32('eyebrowType')
.int32('eyebrowColor')
.int32('eyebrowScale')
.int32('eyebrowAspect')
.int32('eyebrowRotate')
.int32('eyebrowX')
.int32('eyebrowY')
.int32('noseType')
.int32('noseScale')
.int32('noseY')
.int32('mouthType')
.int32('mouthColor')
.int32('mouthScale')
.int32('mouthAspect')
.int32('mouthY')
.int32('beardMustache')
.int32('beardType')
.int32('beardColor')
.int32('beardScale')
.int32('beardY')
.int32('glassType')
.int32('glassColor')
.int32('glassScale')
.int32('glassY')
.int32('moleType')
.int32('moleScale')
.int32('moleX')
.int32('moleY')
.int32('height')
.int32('build')
.string('name', { length: 22, encoding: 'utf-16le', stripNull: true })
.string('creator', { length: 22, encoding: 'utf-16le', stripNull: true })
.int32('gender')
.int32('birthMonth')
.int32('birthDay')
.int32('favoriteColor')
.uint8('favorite')
.uint8('copyable')
.uint8('ngWord')
.uint8('localonly')
.int32('regionMove')
.int32('fontRegion')
.int32('roomIndex')
.int32('positionInRoom')
.int32('birthPlatform')
.buffer('createID', { length: 10 })
.uint16('padding_0')
.int32('authorType')
.buffer('authorID', { length: 8 });
type FFLiCharInfo = b.infer<typeof ffliCharInfoParser>;
const FFLiCharInfo_size = ffliCharInfoParser.size();
function _unpackFFLiCharInfo(u8: Uint8Array, ptr: number): FFLiCharInfo {
return ffliCharInfoParser.parse(u8.subarray(ptr, ptr + FFLiCharInfo_size));
}which keeps all the functionality, brings in type safety, and reduces the size of the section from 147 lines to just 72 (a 48% reduction). For something as big as FFL, I do think the size of the code is something to consider here so that it reduces cognitive noise for those trying to contribute/understand how the library works
Originally I had intended to see if I could get WebGPU working directly, or abstract that layer away in a way that doesn't have a hard tie to ThreeJS so another solution could be used (since at the time, rendering on the server wasn't really possible without some hacks) After I started looking at the implementation though I saw the large number of But you've gotten WebGPU working now as per https://github.com/PretendoNetwork/mii-renderer/issues/1, so that goal isn't much of an issue now. Outside of that, I have no real goals/desires here outside of just trying to improve the tools in the scene. There are a number of I'm fine to close the PR if you'd rather go a different direction and let the library mature on it's own, or just wait until things aren't moving around so rapidly |
|
Ah! I overlooked the huge FFLiCharInfo type. On a serious note, yes. I 100% see what you mean, this is a classic tradeoff between readability and code size. Micro-optimization. Complexity to run versus complexity to write. If I pipe this through Closure Compiler, I get an 18 KB blob of unreadable code soup. I love that. Writing those functions manually is not "convention" that I'd want myself or anyone else to follow. You could probably even call me a hypocrite, after calling out mii-js not using structs, and here I am doing the same lol lol.... My perfect middle ground would be to define structs in human readable code, and generate the serializers. That C/C++ like approach is my favorite, especially if the output code has no dependencies and can be ported to another language easily. dwarfdump for FFLiCharModel
So if I fixed that script, defined all structs in JS, and then built them to an autogenerated untouched file included in the final build, I'd love that. Will I do that- probably not with this project.
Great point here. Here's what's coming to my mind...
Everything you said makes more sense from this perspective, yeah. I guess it's just a shame FFL is the only accurate option right now. If I didn't already mention it, Three.js's abstraction would make it easy to replace this library or write your own.
That's good of you to do. I keep going back and forth about splitting and TS, "more maintainable and less tech debt" vs "smaller and simpler to use" 😔 I guess with this specific project I am "single file C header"-pilled.
🙂 👍 |
2 participants
This started off as just an attempt to split the code based on the
TODOcomments present inffl.js, but then it evolved into a basic TypeScript port as well since it seemed like this library could benefit from real TypeScript supportCompletely untested right now, not even building. Literally just ports the existing JavaScript into TypeScript as 1:1 as possible. The only changes are to get the TypeScript language server in VSCode to shut up, no actual functionality changes have been added
This should absolutely not be merged right now, the old code is left as a reference for now
Changes (some of this might not be in scope for this PR tbf):
TODOcomments about code splitting, all code now lives insrcsplit into the files theTODOcomments namedtsup?)materials. Some of them, likeSampleShaderMaterial, do some funky factory stuff that didn't want to trivially be ported to TypeScript as-is. These will likely require some larger changes to get working@pretendonetwork/eslint-config? I'd hate to dog-food like that and push our standards on another project, but theREADMEdid ask for scrutinization and our eslint config is based off 10+ years of working on Pretendo and other projects). EDIT: I didn't see the eslint config and it wasn't checking thesrcfolder during the port, so I actually hadn't noticed eslint is already here. So this task can probably be removed?updateCharModelwhich turns off the TypeScript checker to updatecharModel._model, which is supposed to bereadonly, and some areas do some odd line splitting and such?)struct-futo the custom parser present here (once it's broken out into it's own module)? Again I'd hate to dog-food like that, butstruct-fudoes seem to have some weird issues with type resolution here, I keep gettingStructInstance<any>results everywhere when doing this port which lacks type safety, and structs have their types defined twice (once instruct-fuand once in JSDoc) which seems to be used for adding intellisense to unpacked data? This ends up taking up a TON of space in the files, especially in cases likeFFLiCharInfo, and the parser I started writing was made with type safety/automatic type generation in mind which would fix that issue