Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct save_as_dds method (textures embedded in NIF) #68

Closed
wants to merge 1 commit into from

Conversation

compomega
Copy link

@niftools/pyffi-reviewer

Overview

Corrects the export of DDS textures from a NIF file using the save_as_dds method (part of ATextureRenderData). This is the first step in fixing embedded texture support in the blender plugin.

Fixes Known Issues

Fixes #3

Documentation

[Overview of updates to documentation]

Testing

[Overview of testing required to ensure functionality is correctly implemented]

Manual

[Order steps to manually verify updates are working correctly]

Automated

[List of tests run, updated or added to avoid future regressions]

Additional Information

[Anything else you deem relevant]

@compomega
Copy link
Author

What would you do for testing? Should some contrived test NIF files be generated that don't contain data from a particular game? So far I've only tested one path through a NIF file from a game I have so it doesn't cover other versions of the format or other pixel formats.

@neomonkeus I noticed you are working on a big refactor of the blender plugin. I don't want to create too big of a merge nightmare - is it getting close?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 57.677% when pulling 9c19b85 on compomega:fix-save_as_dds into 7f4404d on niftools:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 57.677% when pulling 9c19b85 on compomega:fix-save_as_dds into 7f4404d on niftools:develop.

@compomega
Copy link
Author

I have been trying to create an embedded texture from scratch with NIfSkope to create a test case. It's been a bit tricky but I finally have one for the first case (PX_FMT_RGB8). The texture was generated with GIMP:
image

I think this format is supposed to be uncompressed? There is DTX1, DXT3, DXT5, etc. formats as well which are actually compressed. When saving the DDS out of the NIF it was writing a linear size (just the size of the pixel data) instead of a pitch. From my understanding looking at the DDS documentation it should be using a pitch calculation (which is what GIMP is doing in the uncompressed case). Making that change produces the same output DDS file:

#header.flags.linear_size = 1
header.flags.pitch = 1
#header.linear_size = len(self.pixel_data)
header.linear_size = ( header.width * self.bits_per_pixel + 7 ) // 8

Perhaps GIMP is just unforgiving when it comes to the DDS format. What is unfortunate is that the texture must have mipmaps. I don't see any other way to specify the size of the texture in the NIF file. I'll have to see if I can dig up some real world NIF files that have this format.

The usage of pixel_data_matrix was actually removed back in Aug 31, 2008 less than a year after it was added. This looks to be dead code so I'll remove it.

Watch this space 😄

@neomonkeus
Copy link
Member

neomonkeus commented Feb 28, 2020

Firstly, thanks for the PR it looks very clean 👍
The blender plugin pulls the release from pypi, the interface isn't being changed so no worries there.
The code refactor is unaffected even if i merge and publish a new build.

As for test nif there are some test folder which get run as part of the build so feel free to expand on them as much as you think.

Regarding removing pixel_data_matrix we should probably check to see if there are nifs that actually using that feature. Even though a feature may be removed from a specific version of the engine, a game which uses it might still be use the feature, eg Morrowind was released in 2002.

Copy link
Member

@neomonkeus neomonkeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export Embedded Texture fails : save_as_dds : NiPixelData
3 participants