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

Land record table #929

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Pi03k
Contributor

Pi03k commented Apr 19, 2016

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Apr 22, 2016

Contributor

Why the changes to the saving code? Looks random.

Contributor

zinnschlag commented Apr 22, 2016

Why the changes to the saving code? Looks random.

@Pi03k

This comment has been minimized.

Show comment
Hide comment
@Pi03k

Pi03k Apr 22, 2016

Contributor

Small refactoring. I've removed CSMDoc::WriteLandCollectionStage and replaced in with instantiation of template WriteCollectionStage (one class less - code easier to understand/maintain). WriteLandCollectionStage received reference to document class where it really needs only land collection.

Contributor

Pi03k commented Apr 22, 2016

Small refactoring. I've removed CSMDoc::WriteLandCollectionStage and replaced in with instantiation of template WriteCollectionStage (one class less - code easier to understand/maintain). WriteLandCollectionStage received reference to document class where it really needs only land collection.

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Apr 22, 2016

Contributor

It seems the perform function for the Land records is functionally identical to the general perform function of the WriteCollectionStage template (except for the scope check which isn't an issue). Not sure how that happened. But if Land saving is refactored we can as well throw out the specialised function entirely.

Contributor

zinnschlag commented Apr 22, 2016

It seems the perform function for the Land records is functionally identical to the general perform function of the WriteCollectionStage template (except for the scope check which isn't an issue). Not sure how that happened. But if Land saving is refactored we can as well throw out the specialised function entirely.

@Pi03k

This comment has been minimized.

Show comment
Hide comment
@Pi03k

Pi03k Apr 22, 2016

Contributor

Should I remove specialised function and leave default template impl?

Contributor

Pi03k commented Apr 22, 2016

Should I remove specialised function and leave default template impl?

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Apr 22, 2016

Contributor

Looks like that is the way to go. I suggest to do a quick test run with it afterwards (load, save, test in OpenMW). The save code is crucial for OpenMW-CS and any modification to it (no matter how trivial) should be verified through a test.

Contributor

zinnschlag commented Apr 22, 2016

Looks like that is the way to go. I suggest to do a quick test run with it afterwards (load, save, test in OpenMW). The save code is crucial for OpenMW-CS and any modification to it (no matter how trivial) should be verified through a test.

@Pi03k

This comment has been minimized.

Show comment
Hide comment
@Pi03k

Pi03k Apr 22, 2016

Contributor

It looks that vanilla (master) doesn't save Land structure (with merged initial support by @cc9cii ). With my changes situation is the same. I need to debug and fix that. Save doesn't communicate user about any difficulties. Something weird is going on with saving. I'll describe it later.

Contributor

Pi03k commented Apr 22, 2016

It looks that vanilla (master) doesn't save Land structure (with merged initial support by @cc9cii ). With my changes situation is the same. I need to debug and fix that. Save doesn't communicate user about any difficulties. Something weird is going on with saving. I'll describe it later.

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Apr 22, 2016

Contributor

Just tested with master. Loading and saving of land records works fine here.

Contributor

zinnschlag commented Apr 22, 2016

Just tested with master. Loading and saving of land records works fine here.

@Pi03k

This comment has been minimized.

Show comment
Hide comment
@Pi03k

Pi03k Apr 22, 2016

Contributor

How did you test that if there is no land table? Could you please describe your testing procedure?

Contributor

Pi03k commented Apr 22, 2016

How did you test that if there is no land table? Could you please describe your testing procedure?

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Apr 23, 2016

Contributor
  • Load Morrowind.esm into OpenMW-CS
  • Save as Morrowind.omwgame
  • Load Morrowind.omwgame into OpenMW-CS -> terrain renders correctly
  • Load Morrowind.omwgame into OpenMW -> terrain renders correctly
Contributor

zinnschlag commented Apr 23, 2016

  • Load Morrowind.esm into OpenMW-CS
  • Save as Morrowind.omwgame
  • Load Morrowind.omwgame into OpenMW-CS -> terrain renders correctly
  • Load Morrowind.omwgame into OpenMW -> terrain renders correctly
@Pi03k

This comment has been minimized.

Show comment
Hide comment
@Pi03k

Pi03k Apr 25, 2016

Contributor

Could you please elaborate more on 'renders correctly'?
Is step consisting of conversion from esm to omwgame necessary each time?

Contributor

Pi03k commented Apr 25, 2016

Could you please elaborate more on 'renders correctly'?
Is step consisting of conversion from esm to omwgame necessary each time?

@Pi03k

This comment has been minimized.

Show comment
Hide comment
@Pi03k

Pi03k Apr 25, 2016

Contributor

I've run game with modified omwgame and it didn't crash. Doest it mean that terrain renders correctly?

Contributor

Pi03k commented Apr 25, 2016

I've run game with modified omwgame and it didn't crash. Doest it mean that terrain renders correctly?

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Apr 25, 2016

Contributor

Could you please elaborate more on 'renders correctly'?

Looks like normal terrain. I haven't checked if it matches exactly the original but I didn't notice any differences.

Is step consisting of conversion from esm to omwgame necessary each time?

OpenMW-CS can not save in esm format.

Contributor

zinnschlag commented Apr 25, 2016

Could you please elaborate more on 'renders correctly'?

Looks like normal terrain. I haven't checked if it matches exactly the original but I didn't notice any differences.

Is step consisting of conversion from esm to omwgame necessary each time?

OpenMW-CS can not save in esm format.

@Pi03k

This comment has been minimized.

Show comment
Hide comment
@Pi03k

Pi03k Apr 25, 2016

Contributor

I've posted my testing of master openmw-cs on forum
https://forum.openmw.org/viewtopic.php?f=7&t=3453

Contributor

Pi03k commented Apr 25, 2016

I've posted my testing of master openmw-cs on forum
https://forum.openmw.org/viewtopic.php?f=7&t=3453

@ghost ghost changed the title from Feature 936 to Land record table Jun 12, 2017

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 30, 2017

For reference, the last comment in said thread:

You added a land record with an invalid name. That needs to be handled in the LAND table creator bar. The only valid IDs are of the format #x y
Instead of entering an ID string it might be better to have two integer fields for input.

Anyway, the LAND code wasn't written for that. I guess you may have trashed the data structure. In fact the esm/omwgame file format can not store LAND records with such an ID. No idea what kind of junk ended up being written to the file.

So the PR is just missing ID validation? Anything else?

ghost commented Jul 30, 2017

For reference, the last comment in said thread:

You added a land record with an invalid name. That needs to be handled in the LAND table creator bar. The only valid IDs are of the format #x y
Instead of entering an ID string it might be better to have two integer fields for input.

Anyway, the LAND code wasn't written for that. I guess you may have trashed the data structure. In fact the esm/omwgame file format can not store LAND records with such an ID. No idea what kind of junk ended up being written to the file.

So the PR is just missing ID validation? Anything else?

@ghost ghost added the Incomplete label Jul 31, 2017

@akortunov

This comment has been minimized.

Show comment
Hide comment
@akortunov

akortunov Oct 4, 2017

Contributor

Feature 936 is implemented. I think we can close this PR now.

Contributor

akortunov commented Oct 4, 2017

Feature 936 is implemented. I think we can close this PR now.

@ghost ghost closed this Oct 4, 2017

@Pi03k Pi03k deleted the Pi03k:feature_936 branch Nov 11, 2017

@Pi03k Pi03k restored the Pi03k:feature_936 branch Nov 11, 2017

This issue was closed.

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