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

Moved References #557

Merged
merged 23 commits into from
Apr 28, 2015
Merged

Moved References #557

merged 23 commits into from
Apr 28, 2015

Conversation

cc9cii
Copy link
Contributor

@cc9cii cc9cii commented Apr 25, 2015

Feature #2262.

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 25, 2015

Morrowind.esm original

Move the rock via 3D editing from cell (-2 -10) to cell (-2 -9). Save as Morrowind.omwgame then load the new save.

Morrowind.omwgame moved rock

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 25, 2015

Just noticed that I introduced a bug in cellref.cpp.

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 25, 2015

Hmmm.... I think this isn't quite working. I don't see a MVRF record in Morrowind.omwgame.

EDIT: Morrowind.esm has all cells marked as "Added", which means the logic in savingstages that look for non-empty mOriginalCell will not fire.

EDIT2: The same thing happens with Tribunal. A "Base" ref becomes "Modified" when moved, but shows up as "Added" when omwgame is loaded. Need to investigate more.

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 25, 2015

@zinnschlag, should the editing views (references subtable, dialogue as well as 3d editing) automatically update the record's cell data (e.g. mId) when the ref's position changes? At the moment they simply update the x/y/z values. If this is to be done I guess that applies only to external cells.

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 25, 2015

Closing till I make some more progress

@cc9cii cc9cii closed this Apr 25, 2015
@cc9cii cc9cii reopened this Apr 26, 2015
@cc9cii cc9cii changed the title Do not merge - for review only Moved References Apr 26, 2015
@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 26, 2015

Made some changes, please review, especially the saving part. I feel that OpenCS should not generate MVRF as per my forum comments, but if we are going to generate one then it should be done right at the end as per my "workaround".

Loading seems ok, though. Here is an example of NoM 3.0.esm where a rock was moved between cells.

moved ref

And in 3D (the store wasn't rendering before, see forum posts https://forum.openmw.org/viewtopic.php?f=7&t=2606):

cells #-9 7 and -#9 8

@zinnschlag
Copy link
Contributor

Sorry, should have been more specific about that. Loading a move that was made with vanilla TES-CS was always working AFAIK. Seems a lot of things have changed while I was away. Will have a look.

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 26, 2015

Saving and re-loading also works. This is Tribunal.omwgame with a rock moved:

hex editor

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 26, 2015

Loading a move that was made with vanilla TES-CS was always working AFAIK

Yes but the loaded references were placed with the wrong cell (see the changes in data.cpp)

@zinnschlag
Copy link
Contributor

Found a problem. Steps to reproduce:

  • Open the Balmora cells in a scene (-3,-2 to -4, -3)
  • Move the Silt Strider to the other side of the scene.
  • Save

Console output:

std::bad_alloc
Unclosed record remaining

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 26, 2015

I don't get that. Any chance of backtrace or log files please? I won't be able to get to the Ubuntu VM till tomorrow.

balmora

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 26, 2015

Should be fixed.

@zinnschlag
Copy link
Contributor

Better. No more crash. But instead of moving the Silt Strider, it is copied now (the original is still in place after a reload in OpenMW-CS).

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 27, 2015

There are two problems.

First, Data::mRefLoadCache was getting cleared when content file changes. So the ref loaded by content file 0 (Morrowind.esm) was missed by the moved ref loaded by content file 1 (Tribunal.omwgame).

Second, the map search in RefCollection::load() uses mRefNum as the key to find the ref. But because the content file number is different, a cached entry is missed and a new ref is loaded (and now we have two, the original as well as the moved one).

I hacked the files to get things working, but rather ugly. Also, I'm not 100% sure if the moved ref should have the content file number 1 (probably).

EDIT: our content file numbering appears to be different to vanilla. Vanilla refs sometimes have content file number '-2'. Not sure what this means. It doesn't seem to be a relative position, since it occurs for Redesigned Vivec.esp where only Morrowind.esm (content file number 0) is loaded before the mod.

Can't make further progress until I understand how this numbering system works. (reading seems to be ok, but saving is the problem)

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 27, 2015

The content file numbers appear to be our local construct, so ignoring it when looking through the cache.

fixed

@zinnschlag
Copy link
Contributor

Okay. That seems to work. I think this is ready to be merged into master and also ready for the 0.36.0 release. I want more testing anyway and releasing it now seems like the right way to get that. Agreed?

@cc9cii
Copy link
Contributor Author

cc9cii commented Apr 28, 2015

Yes, agreed.

@zinnschlag
Copy link
Contributor

Okay. Will merge. Thanks.

@zinnschlag zinnschlag merged commit 49884f5 into OpenMW:master Apr 28, 2015
return false;
if (esm.isNextSub("MVRF"))
{
if (ignoreMoves)
Copy link

Choose a reason for hiding this comment

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

Isn't this naming backwards? ignoreMoves would suggest when the parameter is true, the MVRF record is skipped, but the code does the opposite.

@cc9cii cc9cii deleted the moveref branch April 28, 2015 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants