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

DIAL, DLBR, INFO, PACK Records & More!!~~ #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

DIAL, DLBR, INFO, PACK Records & More!!~~ #17

wants to merge 1 commit into from

Conversation

Ormin
Copy link

@Ormin Ormin commented Jun 21, 2014

Original commit message:
XOWN ( simple formid ) chunks for CELL, fix to CTDA::VisitFormIDs()
Function_Arguments typo, MODL chunk added to WRLD, GENMNAM is now 28 bytes struct ( up from 16 ), including the 1.8 changes to this struct.

Detailed description:
This was done as a helping API for Skyblivion project, thus API might not be ideal ( I'm not really that familiar with this lib ). It adds some new neat records though, so I suppose you might like to take a look :)

…t ),

XOWN ( simple formid ) chunks for CELL, fix to CTDA::VisitFormIDs()
Function_Arguments typo, MODL chunk added to WRLD, GENMNAM is now 28 bytes struct ( up from 16 ), including the 1.8 changes to this struct.
@lojack5
Copy link
Owner

lojack5 commented Jun 21, 2014

Thanks for the work, I will request a few things from you however:

  • Change your IDE to use 4 spaces instead of tabs for indenting. This is because not every text editor displays tabs as the same size, so you end up with misaligned code depending on which IDE/editor you use. Specifically, you can see this on the GitHub diff pages for your commit.
  • Could you split this up into more focused pull requests? As it is there are a lot of things in there, not all related. It's easier to review and merge separate focused pull requests. I'd suggest:
    • Split each bug-fix into it's own pull request.
    • Split each record type into it's own (although, DIAL and INFO could probably stay together), so:
    • PACK with it's own PR
    • DLBR with it's own PR
    • DIAL with it's own PR (though I can see lumping INFO in there with it, like I said)
    • CELL changes in its own PR
    • etc, etc, I think you get the idea.

Doing this will make it much easier to review, and then I can merge in chunks as I get them reviewed. Not only that, but if I find issue with one part of it, when it's all lumped into one PR, none of it gets merged, instead of just the small PR with the issue.

Other than that nitpicky stuff, the only issue with it I see is the GENMNAM changes: GENMNAM is used by both Oblivion and Fallout: New Vegas in its 16 byte form. So changing that will mess up those two games. The way you want to go about it is to make a new specialized MNAM struct, probably name it SKMNAM, with the updated data, and have Skyrim records use that one instead.

@Ormin
Copy link
Author

Ormin commented Jun 21, 2014

Shouldn't DLBR be incorporated into one pull request with DIALs and INFOs as they are a related topic?

About MNAM, it's okay, I will just create a new structure and revert back MNAM ( even though I was almost certain that even if the structure is bigger, it won't make a difference ).

Tnx :)

@lojack5
Copy link
Owner

lojack5 commented Jun 21, 2014

DLBR: you're right, I forgot that Skyrim added new dialog related record types. Although now we're getting up into quite a bit of code in one PR. I'd prefer split PRs there, but up to you really.

MNAM: for reading in the structure it won't cause a problem for Oblivion/FNV, however when writing out the record it would, since it would end up writing the larger version. Not 100% sure how each of the game engines would handle it: for example Skyrim is pretty resilient when it comes to miss-sized subrecords, but Oblivion would usually crash. Either case though, better to have the tool (CBash) do it correctly than to rely on something like that.

@lojack5 lojack5 added this to the Skyrim Support milestone Jun 21, 2014
@Ormin
Copy link
Author

Ormin commented Jun 22, 2014

Hi,
Unfortunately I don't think I will find time for resplitting this anytime soon ( need to implement QUST support ASAP and to integrate it to our toolchain ), maybe I'll find some time in like two weeks~~. Sorry :(

Will let you guys know when I'll start working on it.

@lojack5
Copy link
Owner

lojack5 commented Jun 22, 2014

That's fine, I'll probably pick out the bug fix and do it manually.

I'd like to say if I get time I'll review the rest and merge it, but more likely I just won't have the time, life's busy.

I will leave the PR open though, so if you ever get back to it you can split things up, or if I find the time I can handle it.

lojack5 added a commit that referenced this pull request Jun 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants