wrye-bash / wrye-bash

A swiss army knife for modding Bethesda games.
https://wrye-bash.github.io
GNU General Public License v3.0
456 stars 79 forks source link

Cell patcher results in ITMs due to water height #302

Closed Arthmoor closed 7 years ago

Arthmoor commented 8 years ago

Hopefully self explanatory in the pic:

https://i.imgur.com/Sgllyfp.jpg

The record should not be getting added to the patch since 0.0000 and the huge negative number effectively mean the same thing.

Further, as this is an interior cell, water height data is irrelevant and can be safely ignored all together. The CK won't even let you set it. So if water height is the only difference, drop the record during processing.

On exterior cells, water height still matters, but if the situation is like the pic, drop the record.

Utumno commented 8 years ago

The value in the pics is -2147483648 (8)

In Oblivion I found this comment in the code:

    #--CS default for water is -2147483648, but by setting default here
    # to -2147483649, we force the bashed patch to retain the value of
    # the last mod.
    MelOptStruct('XCLW','f',('waterHeight',-2147483649)),

https://github.com/wrye-bash/wrye-bash/blob/66d7b4d695289f6dc29142f94a6004494e3306bd/Mopy/bash/game/oblivion/records.py#L535-L538

Not sure how is this default used but could it be intentional they keep the value ? I really don't know

Is processing of waterHeight in interior cells also skipped for Oblivion ? If yes I think I can skip interior cells water height processing right in the patcher - just not positive I should. Even less sure what I should do with -2147483649.

In Skyrim again (some) default value is set to -2147483649 :

https://github.com/wrye-bash/wrye-bash/blob/66d7b4d695289f6dc29142f94a6004494e3306bd/Mopy/bash/game/skyrim/records.py#L1929-L1930

Not sure what this comment means either (sic):

# XCLW sometimes has $FF7FFFFF and causes invalid floatation point
Arthmoor commented 8 years ago

Water height for interior cells can only be skipped for Skyrim right now. I doubt very much NV or FO3 did away with using it, and I know for certain Oblivion DOES use it so it has to be processed.

$FF7FFFFF is basically a ridiculously large number that overflows the signed value they use in that field, so it's invalid when encountered. I can only recall having seen it in Oblivion.esm though.

Utumno commented 8 years ago

Thanks ! If you could attach a couple mods and exact patch options will try and see if I can fix at least interior cells. Will add some overhead and ugly code. However I have to understand why -2147483649

I could check if water height is 0 and then make it -2147483648 and then hope that the patcher will see that all mods have the same value and drop the record - but all these are guesses and seems will add processing overhead

Utumno commented 8 years ago

Well I monkey patched this here: https://github.com/wrye-bash/wrye-bash/tree/utumno-wip

Direct zip: https://github.com/wrye-bash/wrye-bash/archive/utumno-wip.zip

Please test (it may not work - I still need some files that reproduce this to test on my end)

Arthmoor commented 8 years ago

That appears to be at least slightly better, not as many interior cells got pulled in, but there's still one left for those: https://i.imgur.com/JD3qnhB.jpg

I can't pin this down to any one set of files to reproduce it since it just sort of happens.

Also still getting plenty of ITM results on the exterior cells, even when all values agree and there should have been absolutely nothing to patch, like this one: https://i.imgur.com/fkAiEqO.jpg

Utumno commented 8 years ago

That appears to be at least slightly better, not as many interior cells got pulled in, but there's still one left for those: https://i.imgur.com/JD3qnhB.jpg

Well - I don't really know what to do cause I don't yet fully understand what's going on. I could try to change the default to -2147483648 but please then you test carefully cause I have no clue why people have it -2147483649 - deal ? (EDIT: correct link https://github.com/wrye-bash/wrye-bash/blob/66d7b4d695289f6dc29142f94a6004494e3306bd/Mopy/bash/game/skyrim/records.py#L1929-L1930 )

I can't pin this down to any one set of files to reproduce it since it just sort of happens.

Can you link me to the version of the mods in the pic ? Minimal bashed patch options that this happens are also needed.

I ask for those things cause the only way I have to understand what's going on is to run the patcher on the debugger that is line by line. This takes ages, so the less mods and less options the patch has the more possible (as opposed to impossible) this is. At your own pace, please try to edit the other patcher issues you posted with this info if you can pin it - this will make fixing them much faster.

Also still getting plenty of ITM results on the exterior cells, even when all values agree and there should have been absolutely nothing to patch, like this one: https://i.imgur.com/fkAiEqO.jpg

Hmm is this due to water height ? Or other differences ? Note that the Data Size "field" (terminology !) differs - what's that about ?

Quickies:

DianaNites commented 8 years ago

The block and SubBlocks have to do with the file format, as seen here

They're basically a group within a group(within a group), probably to make it easier to get only the relevant data from the file.

The relevant information for CELLs Note the notes it has for XCLW

Utumno commented 8 years ago

Alright thanks - I just needed someone to point me exactly where. Seems that all three floats must be ignored then - I am still shadowboxing in the dark here but keep an eye on my wip branch - may figure something out

Utumno commented 8 years ago

Alright I get all three values - Bash reads them as:

4F7FFFC9 (4294953216.0,)
7F7FFFFF (3.4028234663852886e+38,)
CF000000 (-2147483648.0,)

From what I get from the link by @iaz and from your pics @Arthmoor all these values are equivalent to 0. So if make Bash see them all as 0 would this be enough you think ? Or should I skip those values entirely ? IOW If a previous loading mod tagged with C.Water sets waterHeight to a value and this mod sets waterHeight to one of these values should I overwrite or not ?

Pushed this to my wip branch actually following solution 1 (reading all the values as 0, so overwriting a previous non zero value). Does it work ?

I still need reproducers - point me to mods with the C.Water tag

Utumno commented 8 years ago

@Arthmoor I pushed new commits in my wip branch: https://github.com/wrye-bash/wrye-bash/tree/utumno-wip (direct zip: https://github.com/wrye-bash/wrye-bash/archive/utumno-wip.zip, it's the download zip link in https://github.com/wrye-bash/wrye-bash/tree/utumno-wip)

This should fix interior cells and may reduce duplication in exterior cells too. Not sure about:

Also still getting plenty of ITM results on the exterior cells, even when all values agree and there should have been absolutely nothing to patch, like this one: https://i.imgur.com/fkAiEqO.jpg

Links to mods so I can test this side still appreciated

Re: default water height - I followed the solution to read them all as 0 - does it do the trick ? (exterior, interior should be skipped)

By the way I threw some performance enhancements in.

Please let me know if it fixes the issues reported.

Utumno commented 7 years ago

@Arthmoor: could you see if this is fixed on the latest utumno-wip ?

Arthmoor commented 7 years ago

Sorry it's taken so long to get back to this.

Interior cells for Skyrim are fine. No more need to change anything there.

EXTERIOR cells are bad though. It appears as though you are putting in 0 for any water height value in an exterior cell. This is bad because that will often lead to visible water plains hovering above ground. For these water height that don't need to change you need to adopt the "Default" value. I'm not sure what that is numerically because xEdit won't tell me, but you need to use that rather than 0 or the game will end up with a whole bunch of bad water edits.

Utumno commented 7 years ago

for any water height value in an exterior cell

Any ? I put 0 for 4294953216.0, -2147483648.0,3.4028234663852886e+38 - see my third post above.

What I gathered from your pics is that -2147483648.0 is equivalent to zero - am I wrong ?

I'm not sure what that is numerically because xEdit won't tell me, but you need to use that rather than 0 or the game will end up with a whole bunch of bad water edits.

Yep that was what I wanted to know - this default value :P

Utumno commented 7 years ago

@zilav: what does the "Default" value for water height in xEdit correspond to ? Any of:

4F7FFFC9 (4294953216.0,)
7F7FFFFF (3.4028234663852886e+38,)
CF000000 (-2147483648.0,)
Arthmoor commented 7 years ago

Went and poked through several exteriors in the CK, "Default" appears to correspond to -14000.

I manually changed one of the 0 values to -14000 but xEdit doesn't recognize it as that for some reason. So maybe xEdit has a different value reserved for it.

Utumno commented 7 years ago

OK - you wrote in the OP that:

The record should not be getting added to the patch since 0.0000 and the huge negative number effectively mean the same thing.

And I read from the link by iaz3 http://www.uesp.net/wiki/Tes5Mod:Mod_File_Format/CELL that those numbers correspond to the same thing so I set them all to zero.

Try latest wip - I put all these numbers to -2147483648.0 (not to 0) - still, maybe I should just leave them alone.

Arthmoor commented 7 years ago

Can't you just carry over whatever the "Default" value is? Surely Bash must know since it's processing those records?

zilav commented 7 years ago

Default is 7F7FFFFF, the game will use Default Water Level from worldspace record for such cells hence the name.

m0lz commented 7 years ago

Doesnt the default for Skyrim depend upon the world space setting ( ie it can be different depending upon what world space you are talking about ) http://www.creationkit.com/index.php?title=Worldspace/fr

From what I have read doing a google search I think the general consensus is Skyrim default is -14000 https://forums.nexusmods.com/index.php?/topic/573071-creation-kit-underwater-effects-on-main-land/#entry8177818 https://forums.nexusmods.com/index.php?/topic/678394-tesannwyn-question-about-setting-default-water-heights/#entry5845361

But surely that will depend upon the world space you are in - So I think Wrye Bash for Skyrim ( and probably FO4 etc ) will need to recognise different world spaces, and conflicts to the same records instead of one generic default setting ( maybe why xEdit is not touching that record, because it can be variable, instead of one height which may have been the case in older games such as Oblivion )

Edit: Oops Zilav just said pretty much the same

Arthmoor commented 7 years ago

Thus why I asked if it's not simply possible to carry the default value forward in those cases where it's not being changed :P

Utumno commented 7 years ago

@zilav: thanks - how come 7F7FFFFF (3.4028234663852886e+38) be -14000 is beyond me.

In http://www.uesp.net/wiki/Tes5Mod:Mod_File_Format/CELL we read:

Non-ocean water-height in cell, is used for rivers, ponds etc., ocean-water is globally defined elsewhere.

Is that info right ? Do we need to treat specially 0x4F7FFFC9, 0xCF000000 too ?

@Arthmoor: pushed a new wip version where I set all those numbers to the same one - if this doesn't work right I will just leave any additional processing for exterior cells out.

zilav commented 7 years ago

-14000 is the default water height in Tamriel worldspace set in WRLD record. Just don't change water height in CELLs at all when creating bashed patch, copy it as is.

Arthmoor commented 7 years ago

Except when the value being carried isn't the default of course :P

Arthmoor commented 7 years ago

In any case, yes, your latest WiP has solved the problem of which value to place.

It's generating some ITMs again though for cells with absolutely no changes made.

Utumno commented 7 years ago

In any case, yes, your latest WiP has solved the problem of which value to place.

Thanks! However I still may need to drop processing altogether, so keep testing it

It's generating some ITMs again though for cells with absolutely no changes made.

Maybe due to other record attributes (not water height)? Other patchers ? We should open a new issue for those (once reproducible somehow)

Arthmoor commented 7 years ago

Why would you need to drop processing altogether? Won't that lead to inaccurate results?

No, it's not due to other subrecords. They're definitely ITMs, green all the way across on everything.

Utumno commented 7 years ago

I mean processing of exterior cells default values - I now set all three default values to 7F7FFFFF (3.4028234663852886e+38)

No, it's not due to other subrecords. They're definitely ITMs, green all the way across on everything.

Is that due to water height though ? Or a separate issue ?

In your pic: https://i.imgur.com/fkAiEqO.jpg the data size attribute differs

Arthmoor commented 7 years ago

Got me why the data size differs, because it's different in each file that's listed.

Must be something subtle in what Bash is doing but I couldn't even begin to tell you what. Something xEdit is configured to ignore that has no impact on the game.

zilav commented 7 years ago

xEdit performs two silent edits to all cell records when plugins are loaded: expands DATA flags field to 2 bytes if it is only 1 byte and sets "Default" water level for exterior cells with Has Water flag (XCLW is added if missing).

Data Size can also be affected by different zlib compression levels since the record is compressed.

Utumno commented 7 years ago

xEdit performs two silent edits to all cell records when plugins are loaded: expands DATA flags field to 2 bytes if it is only 1 byte and sets "Default" water level for exterior cells with Has Water flag (XCLW is added if missing).

Aha - thanks!

@Arthmoor - it may be due to some subtle stuff (although zilav's answer does not seem to imply there is something going on we can't see) - but may be due to water level. From the second post here:

In Skyrim again (some) default value is set to -2147483649 :

https://github.com/wrye-bash/wrye-bash/blob/66d7b4d695289f6dc29142f94a6004494e3306bd/Mopy/bash/game/skyrim/records.py#L1929-L1930

Not sure what this comment means either (sic):

# XCLW sometimes has $FF7FFFFF and causes invalid floatation point

So if this default value is used to keep or drop cells it may indeed keep ITMs somehow.

Just not sure where it's used in the code and why people have it 2147483649. Again from the second post here:

In Oblivion I found this comment in the code:

    #--CS default for water is -2147483648, but by setting default here
    # to -2147483649, we force the bashed patch to retain the value of
    # the last mod.
    MelOptStruct('XCLW','f',('waterHeight',-2147483649)),
zilav commented 7 years ago

I'm pretty sure that in Skyrim all abnormal float values like FF7FFFFF, 7F7FFFFF, 4F7FFFC9, CF000000 mean "Default" water level. So anything that is not a valid float value is discarded and water level from WRLD is used instead by the game.

Arthmoor commented 7 years ago

Still getting unnecessary changes to water height as of 307.201610020028.

I still think you should simply copy the record value if no records are changing it. There is no need to switch it from one value to another. I know it seems like stupid nitpicking but it tends to generate records the patch actually doesn't need and people WILL complain about it being full of dirt and wild edits, which will then cause trust issues down the road.

Utumno commented 7 years ago

Alright I will drop all processing I introduced, as anyway I am not perfectly sure For interior cells there should be no duplicate records. For exterior ones I need a minimal load order/patch options - maybe another issue

Utumno commented 7 years ago

@Arthmoor: try latest utumno-wip - interior should be fixed and dropped additional processing for exterior - if exterior is still an issue we need a new issue

Please confirm

Utumno commented 7 years ago

Closed this as it should work for interior cells. Please fill in a new issue for exterior cells as soon as you have a reproducer - the ITMs may be because of some other record attribute

Infernio commented 4 years ago

No such thing as 'wanted ITMs' ;)