search

Marlamin / wow.tools.local

Locally runnable version of the wow.tools website and some of its features
MIT License
97 stars 26 forks source link

Geoset value of 0 sometimes valid #4

Closed WainPetopia closed 1 year ago

WainPetopia commented 1 year ago

Quite a few models have geosets where a value of 0 is a valid appearance, But these aren't included in the geoset selection menus. The menus only have values of 1 or above. I think this is an issue in the modelviewer lib code and not wowdottools (the meshIDs array it returns doesn't seem to include xx00 ones, even if they're valid).

The simplest fix would be to add 0 as a valid option to all geoset config menus. But 0 isn't always a valid setting, and in these cases if you select 0 then you get a hole in the model. But this isn't a huge problem, since the only users who would see that are those using advanced options and presumably know what they're doing. The geoset menus also need to be reset to zero between displayID changes.

Using the above approach I've made changes to my copy that are working for me, but I'm not sure if it's the correct way to go about it.

(Perhaps a more elegant fix would be to add 0 as an option for geosets where some, but not all, displayIDs have entries for that geoset in CreatureDisplayInfoGeosetData (with non-zero values), because I think it can be inferred that 0 is the valid, intended value for all the displayIDs that don't have entries. But that gets way more complicated.)

Just posting this here while I think about the best way to do this, or perhaps you have a better idea. :)

Marlamin commented 1 year ago

If you want to PR your approach that'd be great, I pretty much don't use the modelviewer anymore nor do I have the time to test stuff I don't actively use. :D

WainPetopia commented 1 year ago

modelviewer.diff.txt

Thanks! Does this diff file for modelviewer.js give you what you need, or would you like me to add some context lines around the changes?

WainPetopia commented 1 year ago

I also had a query about line 295 of modelviewer.js: const geosetGroup = Math.round(meshID / 100);

Shouldn't that be Math.floor()? Or maybe it doesn't matter in practice, since geoset values never get high enough for it to round up instead of down?

Marlamin commented 1 year ago

I also had a query about line 295 of modelviewer.js: const geosetGroup = Math.round(meshID / 100);

Shouldn't that be Math.floor()? Or maybe it doesn't matter in practice, since geoset values never get high enough for it to round up instead of down?

I genuinely don't have a proper answer to that without diving back into that which I don't have time for, I'm guessing flooring it makes more sense, but can't verify.

Marlamin commented 1 year ago

I also had a query about line 295 of modelviewer.js: const geosetGroup = Math.round(meshID / 100);

Shouldn't that be Math.floor()? Or maybe it doesn't matter in practice, since geoset values never get high enough for it to round up instead of down?

Send it in a pull request instead if you can.

WainPetopia commented 1 year ago

Ah, that's what "PR" stood for. Sorry. I'll try to get it working in SourceTree and see if I can get a pull request for you.