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

BSLODTriShape and BSFurnitureMarker update #43

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ttl269
Copy link
Member

@ttl269 ttl269 commented Jan 18, 2015

@neomonkeus @skyfox69 @jonwd7 @Ghostwalker71 @throttlekitty @nexustheru @amorilia
BSLODTriShape:

  • Updated description of node itself and all its three specific items (Level 0, 1 and 2).

BSFurnitureMarker:

  • Changed naming/description of all items of BSFurnityreMarker to be more descriptive.
  • Unified name of rotation of marker to "Heading" for both game related Markers.
  • Added Fallout 3 (also selected Oblivion) specific "Marker Type" and "Marker Type Copy" (previously named Position Ref 1 and 2) definitions in "FurnitureMarkerType" enum.

Above changes in BSFurnitureMarker breaks Nifskope's (1.2a2 version) to draw Furniture markers. It would require to update function drawFurnitureMarker in glnode.cpp

BSLODTriShape:
Updated description of node itself and all its three specific items (Level 0, 1 and 2).

BSFurnitureMarker:
- Changed naming/description of all components of BSFurnityreMarker to be more descriptive. Instead of "FurniturePositions" is used "FurnitureMarker" etc.
- Unified name of rotation of marker to "Heading" for both game related Markers.
- Added Fallout 3 specific "Marker Type" and "Marker Type Copy" (previously named Position Ref 1 and 2) definitions in "FurnitureMarkerType" enum.
Updated conditions for working with Oblivion nifs too.
@ttl269 ttl269 changed the title Feature/BSLODTriShape and BSFurnitureMarker update BSLODTriShape and BSFurnitureMarker update Jan 18, 2015
@hexabits
Copy link
Member

@ttl269 Good update, but now more than ever I need to look into XML version checking. :) In NifSkope I will need to support a bare minimum XML version and when I make changes to NifSkope for breaking changes such as this, I will need to either branch or deprecate the behavior based on the XML version. Though changes such as this are so major I would just need to raise the minimum required version, and alert the user when they are using an unsupported XML version.

So with that said, I'll ask that when you make breaking changes you increment the minor version (0.7.1.0 > 0.7.2.0). Or however you would prefer, as long as I can detect the changes based on the XML version.

@ttl269
Copy link
Member Author

ttl269 commented Jan 19, 2015

@jonwd7 I understand. It is not problem to increment minor version of xml.nif in case of doing breaking changes. To make it clear - by breaking change you mean a change which really breaks Nifskope's behavior somehow? Not breaking in meaning of super new cool feature :)
In recent Pull requests there are two of them doing that break - this and #39 (breaks rendering of Constraints). I always write it in comment, but it is only notice for you.
So, I will add next commit to both of these branches with xml version increment to 0.7.2.0, so after merging all pull requests the version should be 0.7.2.0. Am I right?

BTW: I really appreciate your input on niftools forum regarding proposals of changes to XML format - there is so much ideas. I am preparing my answer but it will take me some time to assemble it. But saying it now - I agree with most of proposals. Right for this furniture break case, having IDs as you proposed, would be helpful, because Nifskope would rely on ID, not on name or displayname of item.

@hexabits
Copy link
Member

for this furniture break case, having IDs as you proposed, would be helpful, because Nifskope would rely on ID, not on name or displayname of item.

Exactly.

by breaking change you mean a change which really breaks Nifskope's behavior somehow? Not breaking in meaning of super new cool feature

Well, the new cool feature breaks. So anyone using the dev builds (1.2, 2.0) that relies on seeing the furniture markers will find it broken when switching to the new XML (if I don't update the code first).

Most simply I would say that changing any text inside a name attribute is one type of a breaking change. Assuming there is a program out there that relies on it.

So, I will add next commit to both of these branches with xml version increment to 0.7.2.0, so after merging all pull requests the version should be 0.7.2.0. Am I right?

Sounds good. Lumping lots of breaking changes into one version change is a good idea. Less versions to keep track of for whatever versioning system I make.

Increased niftoolsxml version to 0.7.2.0 due to chages in names in this branch.
@ttl269
Copy link
Member Author

ttl269 commented Jan 20, 2015

Most simply I would say that changing any text inside a name attribute is one type of a breaking change. Assuming there is a program out there that relies on it.

That is true. And because nobody can't know which program rely on which name, any change in name attribute should lead to xml version increment.
Same case is change in structure, for example splitting previous one add into more add items or vice versa.

I have found that all five recent pull requests makes breaking changes - in names or even in structure, so all of them got XML version update to same new value 0.7.2.0.

@neomonkeus
Copy link
Member

As usual I have very little to contribute that hasn't already been discussed.
There are a few options:

  • Prioritise the pull requests. Pick one pull request, update the version number on that one first. Rebate the next highest. (perhaps first one is least amount of Nifskope changes involved. allow incremental support. Easier to merge to dev.)
  • Update all branches to new version (Require all Nifskope changes in go?)
  • I forgot the third suggestion....

@neomonkeus neomonkeus modified the milestone: Version 0.8 Apr 12, 2016
@neomonkeus neomonkeus modified the milestones: Version 0.8, Version 1.0 Sep 18, 2019
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.

3 participants