-
Notifications
You must be signed in to change notification settings - Fork 114
Pyffi overhaul #535
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
Pyffi overhaul #535
Conversation
…er flags options/setting.
|
Could we change the import to put the .classes part into the import statements instead of each NifFormat call? Not sure if that works though |
Yes. I just did it. |
…rtitions triangles.
HENDRIX-ZT2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few remarks, one important todo. Stellar job so far!
io_scene_niftools/modules/nif_export/property/shader/__init__.py
Outdated
Show resolved
Hide resolved
io_scene_niftools/modules/nif_export/property/texture/types/bsshadertexture.py
Outdated
Show resolved
Hide resolved
io_scene_niftools/modules/nif_import/property/shader/__init__.py
Outdated
Show resolved
Hide resolved
io_scene_niftools/modules/nif_import/property/shader/bsshaderlightingproperty.py
Outdated
Show resolved
Hide resolved
io_scene_niftools/modules/nif_import/property/shader/bsshaderproperty.py
Outdated
Show resolved
Hide resolved
io_scene_niftools/modules/nif_import/property/texture/loader.py
Outdated
Show resolved
Hide resolved
…andled by the file reading lib.
…hain suggestions from Hendrix' PR review.
…ecessary with the new xml.
…tShaderProperty shader type setting.
…e was the same. Fixed issues that cropped up with importing testing nif/kf.
… to generated library.
|
Hello. I've been keeping an eye on this project as I'm interested in it. May I ask if there is an update on this PR? I was motivated to comment because, when looking at open PRs at the PyFFI repository, I saw niftools/pyffi#74, which looks, at least, like a similar situation to this PR. It has one reviewer giving it an OK, with another requested to review, and seems to have remained in that state for 2 years. I feel it would be a shame for this PR to end up in a similar situation. The contributor might leave when they could have instead been motivated to continue sending in additional improvements if the PR was accepted. So, this is just to let you know that those like myself who are interested in the project, and either a merge of PRs like this or the one I linked to, or a comment about why merging is not yet possible, etc. would be appreciated and keep interest going. Thanks. Edit: Looks like the contributor is a member of Niftools, so what I said about them possibly leaving may not be applicable. |
Seems like Neomonkeus (who normally does the merging/releases) isn't available at the moment. My plan so far is to wait until the 19th and merge it myself if there hasn't been any update by then. In the meantime I'm still working on the addon, I'm just not yet pushing the commits to GitHub. |
|
Thanks for the update. I didn't know you were able to merge it yourself (I didn't realize you were part of the Niftools team either until I looked at your profile after making my comment), and it's good to hear that work is continuing. It'll be great if we can start importing and exporting .nif files with current versions of Blender, which doesn't seem to work yet (I've only tested with Oblivion .nif files, so I don't know if other games work). |
Importing and exporting nif files with current versions of Blender should already be possible with the released versions of the plugin (especially Oblivion nifs, which have long been supported) - if you're encountering errors while doing that, please make an issue and post the concerning nifs so that we can help. |
|
The problems I encountered are covered by #453. Trying to import meshes/architecture/anvil/anvilaltar01.nif failed because of "'int' object has no attribute 'material'" errors. A fix is mentioned in that issue report, and it did allow the import to proceed, but I don't know if it's a correct fix. The exported .nif then supposedly lacks collision in-game. I can't remember if I tested that part myself, but I was left with the impression that the plugin wasn't functional yet, at least for Oblivion .nif files. |
|
Sorry, I remembered the problem incorrectly. The "material" error happens on export, not import, and it still occurs. I tested this with the latest development code (newer than the release code, which fails to export with an element-wise multiplication error that has since been fixed). |
|
I remembered I had this conversation niftools/pyffi#69 over at the PyFFI project, where I was told the "material" workaround wasn't a good solution (it would "break other things"). I was told you and HENDRIX-ZT2 were working on a proper solution. So that's where I got the impression things were WIP and it would be some time before Oblivion .nifs could be exported successfully. I don't know how common the "material" issue is with Oblivion .nifs, but testing a few others I quickly came upon another with the same error: meshes/architecture/anvil/anvilfgfirstfloor.nif. Anyway I didn't mean to get off-topic. I know this PR is not specifically targeting the issue I mentioned, but it looks like it may be the proper solution I was told about, so I'm hopeful it will fix it. |
|
I think we can just go ahead and merge this. IMO we don't need to do micro-releases after every PR. It just clogs down development. |
|
What is the 'generated' module actually? Is it missing directory to be pushed? |
It's a module that is generated using codegen.py from cobra-tools: https://github.com/OpenNaja/cobra-tools using this code: https://github.com/Candoran2/new-pyffi as source and the updated version of nifxml: https://github.com/Candoran2/nifxml/tree/develop . If you have any questions about it, you're free to ask them here, but the NifMania discord would also work for (likely) somewhat more instant communication. |
|
Thank you for the reply. Now it works. |
|
@Candoran2 Could you explain a bit more how to create the needed module? I've downloaded the latest cobra-tools and followed the instructions to install Python and run "pip install pyqt5 imageio". I've also downloaded the latest sources from https://github.com/Candoran2/new-pyffi and https://github.com/niftools/nifxml/tree/develop (which seems to be newer than https://github.com/Candoran2/nifxml/tree/develop), but I don't know how to proceed and can't find any information on using codegen with pyffi. |
For simple operation: Copy or softlink the Then, simply run codegen.py. There's a lot of other code in the cobra-tools repository, but that and the codegen folder are the only part that we need (and some other base format libraries like base and dds that get generated similarly). This will fill the |
|
Thank you for the instructions. I was able to get the plugin working (with problems, see below) by running With only the While the addon is at least working to some degree, I'm seeing errors and actually get somewhat less far in my test case ( I'm testing two versions of anvilaltar01.nif. One is the version you get from installing from GOG or Steam. The other is from the DVD release (the DVD release has slightly different mesh files that report a later NIF version). With the v0.014 addon both versions of anvilaltar01.nif import successfully. They both fail to export due to the problems mentioned in #453. With the newer addon code from the develop branch I'm trying now, only the GOG/Steam version imports successfully. The DVD version fails with The GOG/Steam version successfully imports but has an error When trying to export, it fails with the following Any ideas? |
|
So the generated module is indeed in addition to the existing dependencies, you can't remove the old ones quite yet. The first error is indeed fixed in #541, not sure about the second one. It very well might be fixed there (I did make some updates for collision) but it's possible I missed some places. As for export, I haven't made any huge changes there. Just note that, since Oblivion has a lot of nif versions, it's probably safest to set the game yourself right before exporting. |
|
I updated to Blender 3.4 (I doubt that makes a difference) and am testing the newer code from #541 now. I can confirm the DVD version of anvilaltar01.nif imports now, although it has the same After import, the DVD version of anvilaltar01.nif has game set to The GOG/Steam .nif also imports (with the above-mentioned error), but the game is automatically set to Now time to go check the exported .nifs in-game. |
|
Both of the exported .nifs crash Oblivion when I try to load a saved game where I'm standing in front of the altar model. (I tried replacing the .nif with another .nif that comes with the game and that did not crash, so I think the crash is being caused by the exported .nif files) Edit: Just to be sure I tried starting a new game (rather than loading a saved game) and warping to the cell with the .nif with the console, and the crash still happened. |
Probably best to start an issue at this point. |
|
Alright, I'll do so. Thanks again for helping me get things up and running so I could test the new code. |
|
Looks like you've made some fixes, so I'll re-build and re-test before making any issues. |
@niftools/blender-niftools-addon-reviewer
Overview
Detailed Description
generated.formats.nifin theclassesvariable (it's actually a dict that also allows value access throughdict.keyin addition todict['key']).skin_partitionproperty as alternative access to theskinfield because it pretty much refers to the same thing as theskin_partitionfield on NiGeometry. Ideally this would be adjusted in the xml so it isn't necessary, but that would probably be a breaking change for other software that use the xml.Fixes Known Issues
Documentation
[Overview of updates to documentation]
Testing
[Overview of testing required to ensure functionality is correctly implemented]
Manual
[Set of 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]