InfoTranslator: Draft randomUnits support#113
Conversation
ChiefOfGxBxL
left a comment
ChiefOfGxBxL
left a comment
There was a problem hiding this comment.
Thanks for the contribution. A bunch of this work is actually in the TypeScript version already, including some of your previous work!
| // Partial support: Unit table (random) | ||
| outBuffer.addInt(infoJson.randomUnits.length); | ||
| for (const randomUnitsGroup of infoJson.randomUnits) { | ||
| outBuffer.addInt(randomUnitsGroup.id); | ||
| outBuffer.addString(randomUnitsGroup.name, true); | ||
| outBuffer.addInt(randomUnitsGroup.subTables.length); | ||
| for (let i = 0; i < randomUnitsGroup.subTables.length; i++) outBuffer.addInt(0); // ???? | ||
| outBuffer.addInt(randomUnitsGroup.subTables.length); | ||
| for (let i = 0; i < randomUnitsGroup.subTables.length; i++) { | ||
| outBuffer.addFourCCTwice(randomUnitsGroup.subTables[i].variants.length); | ||
| for (let j = 0; j < randomUnitsGroup.subTables[i].variants.length; j++) { | ||
| outBuffer.addString(randomUnitsGroup.subTables[i].variants[j].object, false); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It looks like you're editing a .js file, when the project had several years ago been ported to TypeScript (.ts). The random unit/item table is already implemented via https://github.com/ChiefOfGxBxL/WC3MapTranslator/blob/master/src/translators/InfoTranslator.ts#L728-L757.
There was a problem hiding this comment.
Yeah. After submitting this, I realized that v5.0.0 already supports random units tables. I was under the impression that v4.0.0 didn't and that v5.0.0 didn't either, which is why I originally thought this PR would be more useful 😅 , but I was wrong lol.
Still, there are some details here such as UTF8 strings, which I do correctly, but the rest of my algorithm is actually iffy.
| return String.fromCharCode(ch); | ||
| }).join(''); | ||
| }, | ||
| readChars: function(len, allowNull = false) { |
There was a problem hiding this comment.
Likewise, allowNull is also already supported: https://github.com/ChiefOfGxBxL/WC3MapTranslator/blob/master/src/W3Buffer.ts#L49
There was a problem hiding this comment.
Yeah, I know. As I said, this is based on v1.0.0, so I had to include it for completeness.
Ultimately, gotta evaluate to what extent each implementation of random tables is accurate. It's a bit awkward that I didn't realize at first that you already had an implementation... Feel free to close and/or cherry-pick improvements.
| }, | ||
|
|
||
| addFourCCTwice: function(int) { | ||
| if (int >= 10) throw new Error(`idk how to encode ${int}`); |
There was a problem hiding this comment.
encoding an int can be done via HexBuffer.addInt
There was a problem hiding this comment.
This method is roughly supposed to store an integer as a decimal string, not as an int properly.
| let c = string.charCodeAt(i); | ||
| if (c === 0) continue; | ||
| if (c < 0x30 || 0x39 < c) throw new Error(`Invalid numerical FourCC`); | ||
| value += (c - 0x30) * (1 << (8 * i)); |
There was a problem hiding this comment.
This is interesting - I haven't seen documentation on WC3's FourCC format, so am unfamiliar.
Do you have any references or examples, perhaps ones where the prior WC3MapTranslator implementation fails and this one is correct?
There was a problem hiding this comment.
Do you have any references or examples, perhaps ones where the prior WC3MapTranslator implementation fails and this one is correct?
Will gather this. Note that the previous "FourCC" implementation is correct per se. This is a different thing, which I named "FourCC twice" for lack of a better name.
There was a problem hiding this comment.
Also, regarding FourCC, this is a close reference https://ubershmekel.github.io/fourcc-to-text/. The difference is that WC3 treats FourCC texts as case-insensitive in some contexts, such as tooltips files. I haven't fully researched the sensitiveness stuff, but it's not relevant for WC3MapTranslator.
2 participants
w3ifile format 25 (TFT)master.