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

Editor: Land and Land Texture Tables #1427

Merged
merged 25 commits into from Oct 3, 2017

Conversation

Projects
None yet
5 participants
@Aesylwinn
Contributor

Aesylwinn commented Sep 1, 2017

This is what I've got so far. Basic table functionality is implemented for the LTEX tables. You can clone/add LTEX records and change the name/texture. You can also "touch" LAND records, so that they will use the current plugin land textures. Viewing the changes in the scene widgets does not work yet, but the effects can be seen by running openmw with the addon loaded.

Forum post
LTEX feature
LAND feature

@Aesylwinn Aesylwinn changed the title from [Do Not Merge Yet] Land and Land Texture Tables to [Do Not Merge Yet] Editor: Land and Land Texture Tables Sep 1, 2017

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 4, 2017

Contributor

Loading land data later doesn't play nicely with the editor at the moment. The land load code may need some significant refactoring.

Contributor

Aesylwinn commented Sep 4, 2017

Loading land data later doesn't play nicely with the editor at the moment. The land load code may need some significant refactoring.

Aesylwinn added some commits Sep 4, 2017

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 6, 2017

Contributor

My current list of what remains to be done:

  1. Figure out the best way to solve the problem with land data delayed loading.
  2. Connect and handle signals to update the terrain in the scene view.
  3. Import land textures when cloning lands.
  4. Add the ability to merge multiple land textures into one.

I'll be taking a break from this for a few days.

Contributor

Aesylwinn commented Sep 6, 2017

My current list of what remains to be done:

  1. Figure out the best way to solve the problem with land data delayed loading.
  2. Connect and handle signals to update the terrain in the scene view.
  3. Import land textures when cloning lands.
  4. Add the ability to merge multiple land textures into one.

I'll be taking a break from this for a few days.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 8, 2017

Contributor

I'm getting some strange artifacts with the terrain system. I think it may have to do with colors.
color_artifacts.png

Contributor

Aesylwinn commented Sep 8, 2017

I'm getting some strange artifacts with the terrain system. I think it may have to do with colors.
color_artifacts.png

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 8, 2017

Contributor

Seems to only be an issue with cloned/touched lands which both copy the data.

Contributor

Aesylwinn commented Sep 8, 2017

Seems to only be an issue with cloned/touched lands which both copy the data.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 8, 2017

Contributor

Turned out to be an array bounds issue.

Contributor

Aesylwinn commented Sep 8, 2017

Turned out to be an array bounds issue.

@Aesylwinn Aesylwinn changed the title from [Do Not Merge Yet] Editor: Land and Land Texture Tables to Editor: Land and Land Texture Tables Sep 9, 2017

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 9, 2017

Contributor

Land texture merging can be implemented later with the automatic land texture deletion feature, since both will require indexing the textures used in each land texture.

Contributor

Aesylwinn commented Sep 9, 2017

Land texture merging can be implemented later with the automatic land texture deletion feature, since both will require indexing the textures used in each land texture.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 10, 2017

Contributor

Just a heads up, I'm likely to lose power/internet access at some point tomorrow, and it may take a few days to come back up, so I may not respond to feedback in a reasonable amount of time. Also, I'll admit I was rushing this toward the end, so I would suggest taking time with the reviewing process, especially in loadland.cpp and columnimp.cpp. I'll review it myself in a day or two assuming I'm able to.

Contributor

Aesylwinn commented Sep 10, 2017

Just a heads up, I'm likely to lose power/internet access at some point tomorrow, and it may take a few days to come back up, so I may not respond to feedback in a reasonable amount of time. Also, I'll admit I was rushing this toward the end, so I would suggest taking time with the reviewing process, especially in loadland.cpp and columnimp.cpp. I'll review it myself in a day or two assuming I'm able to.

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Sep 10, 2017

Contributor

No hurry. Extremely busy right now, so I won't get around to do the review for a couple of days. Entirely possible that you will beat me to it with your own final review.

Contributor

zinnschlag commented Sep 10, 2017

No hurry. Extremely busy right now, so I won't get around to do the review for a couple of days. Entirely possible that you will beat me to it with your own final review.

@psi29a

This comment has been minimized.

Show comment
Hide comment
@psi29a

psi29a Sep 10, 2017

Member

I'm so glad you have taken this task on. I'll review this as well in the coming days.

Member

psi29a commented Sep 10, 2017

I'm so glad you have taken this task on. I'll review this as well in the coming days.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 10, 2017

Contributor

Alright, take your time.

Contributor

Aesylwinn commented Sep 10, 2017

Alright, take your time.

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Sep 17, 2017

Contributor

Here is my first review pass. Besides a couple of minor issues that I commented on inline I see one major problem. In the LandTexture table there are multiple records with the same texture index (sometimes even more than 2). That is confusing to the user and is not useful. For all intents and purposes LTEX records that are not part of the currently edited plugin don't exist.
Furthermore when I touched a Land record from base, the resulting LandTexture records had very high texture indices (4 digits or so, while everything else seems to be barely above 100. That looks odd and may require further investigation.

Contributor

zinnschlag commented Sep 17, 2017

Here is my first review pass. Besides a couple of minor issues that I commented on inline I see one major problem. In the LandTexture table there are multiple records with the same texture index (sometimes even more than 2). That is confusing to the user and is not useful. For all intents and purposes LTEX records that are not part of the currently edited plugin don't exist.
Furthermore when I touched a Land record from base, the resulting LandTexture records had very high texture indices (4 digits or so, while everything else seems to be barely above 100. That looks odd and may require further investigation.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 17, 2017

Contributor

In the LandTexture table there are multiple records with the same texture index (sometimes even more than 2). That is confusing to the user and is not useful.

Should we just not show Base LandTextures then?

Furthermore when I touched a Land record from base, the resulting LandTexture records had very high texture indices (4 digits or so, while everything else seems to be barely above 100.

When importing textures that conflict with existing indices (and have a different texture or texture handle/name), a new index is chosen. I spread out these indices because it should reduce the number of future conflicts (when selecting new indices) and I saw no downsides.

Contributor

Aesylwinn commented Sep 17, 2017

In the LandTexture table there are multiple records with the same texture index (sometimes even more than 2). That is confusing to the user and is not useful.

Should we just not show Base LandTextures then?

Furthermore when I touched a Land record from base, the resulting LandTexture records had very high texture indices (4 digits or so, while everything else seems to be barely above 100.

When importing textures that conflict with existing indices (and have a different texture or texture handle/name), a new index is chosen. I spread out these indices because it should reduce the number of future conflicts (when selecting new indices) and I saw no downsides.

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Sep 18, 2017

Contributor

Should we just not show Base LandTextures then?

That sounds reasonable.

When importing textures that conflict with existing indices (and have a different texture or texture handle/name), a new index is chosen. I spread out these indices because it should reduce the number of future conflicts (when selecting new indices) and I saw no downsides.

Fair enough. But have you considered the case of multiple editing operations that require creating new LTEX indices. If this is done again and again over multiple content files, would this trick still help with avoiding conflicts and are we at risk of running out of indices eventually.

Contributor

zinnschlag commented Sep 18, 2017

Should we just not show Base LandTextures then?

That sounds reasonable.

When importing textures that conflict with existing indices (and have a different texture or texture handle/name), a new index is chosen. I spread out these indices because it should reduce the number of future conflicts (when selecting new indices) and I saw no downsides.

Fair enough. But have you considered the case of multiple editing operations that require creating new LTEX indices. If this is done again and again over multiple content files, would this trick still help with avoiding conflicts and are we at risk of running out of indices eventually.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 23, 2017

Contributor

Not sure if their is a good way to hide the base records. I have made it so they appear grayed out for now.

For containers, I've chose to use a QVector over an std::vector because the QVector should work better with how QVariants work (lots of copies). It would not be difficult to change though.

The performance using a map for texture inputs was decent. On my laptop with an i5-5200u processor it takes 1-2 seconds to touch all land records with Morrowind, Tribunal, and Bloodmoon loaded.

Should the TextureHandle column (which I renamed to TextureNickname) be removed or kept?

Contributor

Aesylwinn commented Sep 23, 2017

Not sure if their is a good way to hide the base records. I have made it so they appear grayed out for now.

For containers, I've chose to use a QVector over an std::vector because the QVector should work better with how QVariants work (lots of copies). It would not be difficult to change though.

The performance using a map for texture inputs was decent. On my laptop with an i5-5200u processor it takes 1-2 seconds to touch all land records with Morrowind, Tribunal, and Bloodmoon loaded.

Should the TextureHandle column (which I renamed to TextureNickname) be removed or kept?

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 28, 2017

Contributor

Are the textures always case insensitive regardless of the filesystem? I noticed that there are some duplicate textures as a result of having a different case, and if that never matters then we should ignore the case. If it does matter, then there is an issue with the auto completion which seems to always suggests lower case names.

Contributor

Aesylwinn commented Sep 28, 2017

Are the textures always case insensitive regardless of the filesystem? I noticed that there are some duplicate textures as a result of having a different case, and if that never matters then we should ignore the case. If it does matter, then there is an issue with the auto completion which seems to always suggests lower case names.

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Sep 28, 2017

Contributor

Everything in Morrowind is case-insensitive (stupid Windows!). If OpenMW treats anything non case-sensitive then that is a bug.

Contributor

zinnschlag commented Sep 28, 2017

Everything in Morrowind is case-insensitive (stupid Windows!). If OpenMW treats anything non case-sensitive then that is a bug.

@psi29a

This comment has been minimized.

Show comment
Hide comment
@psi29a

psi29a Sep 28, 2017

Member

wait, what? Shouldn't that read:
If OpenMW treats anything case-sensitive then that is a bug.

Everything should be case-insensitive, thanks to being handicapped by Windows. Right?

Member

psi29a commented Sep 28, 2017

wait, what? Shouldn't that read:
If OpenMW treats anything case-sensitive then that is a bug.

Everything should be case-insensitive, thanks to being handicapped by Windows. Right?

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Sep 28, 2017

Contributor

Oops. Yes, you are right. The last sentence was incorrect.

Contributor

zinnschlag commented Sep 28, 2017

Oops. Yes, you are right. The last sentence was incorrect.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Sep 30, 2017

Contributor

I'm getting some errors in the console, but I think they are the result of recent changes and not mine.

Error: RigGeometry rendering with no skeleton, should have been initialized by UpdateVisitor
before Glyph::subload(): detected OpenGL error: invalid value
Warning: detected OpenGL error 'invalid value' at after RenderBin::draw(..)

Contributor

Aesylwinn commented Sep 30, 2017

I'm getting some errors in the console, but I think they are the result of recent changes and not mine.

Error: RigGeometry rendering with no skeleton, should have been initialized by UpdateVisitor
before Glyph::subload(): detected OpenGL error: invalid value
Warning: detected OpenGL error 'invalid value' at after RenderBin::draw(..)

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Oct 1, 2017

Contributor

I get the same errors on the master branch. Regarding the hiding of base LandTexture records, perhaps that could done using the existing search/sort mechanisms?

Contributor

Aesylwinn commented Oct 1, 2017

I get the same errors on the master branch. Regarding the hiding of base LandTexture records, perhaps that could done using the existing search/sort mechanisms?

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Oct 2, 2017

Contributor

Regarding the hiding of base LandTexture records, perhaps that could done using the existing search/sort mechanisms?

Don't see how that works since what is displayed in the table is not determined by search/sort. User a filter may be an option if it doesn't interfere with other filtering functions. The only other option I see is to manually change the mapping between records in the collections and rows in the table model.

Contributor

zinnschlag commented Oct 2, 2017

Regarding the hiding of base LandTexture records, perhaps that could done using the existing search/sort mechanisms?

Don't see how that works since what is displayed in the table is not determined by search/sort. User a filter may be an option if it doesn't interfere with other filtering functions. The only other option I see is to manually change the mapping between records in the collections and rows in the table model.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Oct 2, 2017

Contributor

The only other option I see is to manually change the mapping between records in the collections and rows in the table model.

I don't think we want to do that since we only want to hide the records from the user, and the tables can be accessed for other uses. I'll see if I can figure out a way to make filtering work.

Contributor

Aesylwinn commented Oct 2, 2017

The only other option I see is to manually change the mapping between records in the collections and rows in the table model.

I don't think we want to do that since we only want to hide the records from the user, and the tables can be accessed for other uses. I'll see if I can figure out a way to make filtering work.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Oct 2, 2017

Contributor

I think everything has been addressed now.

Contributor

Aesylwinn commented Oct 2, 2017

I think everything has been addressed now.

@zinnschlag

This comment has been minimized.

Show comment
Hide comment
@zinnschlag

zinnschlag Oct 3, 2017

Contributor

I'll merge that for now. I don't see anything wrong with it, but we definitely need more testing.

I think we still need to think over the "texture nickname" column. It does not make any sense to me. And AFAIK this field isn't used anywhere. Maybe we should just hide it? Or maybe get some input from other modders on the forum first?

With this done the next task will be terrain texture selection. @PlutonicOverkill has done some work on it but I don't know how complete it is. @Aesylwinn Do you want to continue to work on the terrain feature? Unless PlutonicOverkill wants to go on with it, maybe you could take that over his work (#1414).

Contributor

zinnschlag commented Oct 3, 2017

I'll merge that for now. I don't see anything wrong with it, but we definitely need more testing.

I think we still need to think over the "texture nickname" column. It does not make any sense to me. And AFAIK this field isn't used anywhere. Maybe we should just hide it? Or maybe get some input from other modders on the forum first?

With this done the next task will be terrain texture selection. @PlutonicOverkill has done some work on it but I don't know how complete it is. @Aesylwinn Do you want to continue to work on the terrain feature? Unless PlutonicOverkill wants to go on with it, maybe you could take that over his work (#1414).

@zinnschlag zinnschlag merged commit 2f5449a into OpenMW:master Oct 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@psi29a

This comment has been minimized.

Show comment
Hide comment
@psi29a

psi29a Oct 3, 2017

Member

\o/

This is great news for the next release!

I think we'll need some tutorials for how to use this. :) Or in the next video, that this is demoed!

Member

psi29a commented Oct 3, 2017

\o/

This is great news for the next release!

I think we'll need some tutorials for how to use this. :) Or in the next video, that this is demoed!

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Oct 3, 2017

Contributor

I've gone ahead and posted a thread on the forums asking for opinions about the unused field. I'm somewhat busy at the moment, so I probably wouldn't be able to continue with the LAND/LTEX related features for at least a month, if PlutonicOverkill does not wish to continue with it.

@psi29a I'm afraid this was more grounds work for terrain editing, so there isn't much you can/should do with it. Touching land records and changing the textures referred to should not be preferred over creating a texture with the same name for mod compatibility reasons.

Contributor

Aesylwinn commented Oct 3, 2017

I've gone ahead and posted a thread on the forums asking for opinions about the unused field. I'm somewhat busy at the moment, so I probably wouldn't be able to continue with the LAND/LTEX related features for at least a month, if PlutonicOverkill does not wish to continue with it.

@psi29a I'm afraid this was more grounds work for terrain editing, so there isn't much you can/should do with it. Touching land records and changing the textures referred to should not be preferred over creating a texture with the same name for mod compatibility reasons.

@PlutonicOverkill

This comment has been minimized.

Show comment
Hide comment
@PlutonicOverkill

PlutonicOverkill Oct 4, 2017

Contributor

Hi, I am moving house next week, and since this is a very busy time of year, I can't do any work on the terrain selection feature during the next few weeks. I've got OSG built, but still haven't managed to get everything correctly linked and running, and until that happens I won't be able to continue with it. I also have to admit, I get a bit lost whenever a discussion comes up surrounding LAND and LTEX records, so I'll need to research those properly as well.

Contributor

PlutonicOverkill commented Oct 4, 2017

Hi, I am moving house next week, and since this is a very busy time of year, I can't do any work on the terrain selection feature during the next few weeks. I've got OSG built, but still haven't managed to get everything correctly linked and running, and until that happens I won't be able to continue with it. I also have to admit, I get a bit lost whenever a discussion comes up surrounding LAND and LTEX records, so I'll need to research those properly as well.

@Aesylwinn

This comment has been minimized.

Show comment
Hide comment
@Aesylwinn

Aesylwinn Oct 4, 2017

Contributor

The files components/esm/loadland.hpp and components/esm/loadltex.hpp are good places to start. I believe there is a small discussion about LTEX records in the forum post linked in the opening post. You probably also want to look at apps/opencs/model/world/columnimp.hpp for the way to access and set the relevant data. Looking at how this PR "imports" LTEX records would probably also be beneficial. (files are off the top of my head, hopefully they are correct).

Seeing how PlutonicOverkill wants to continue, I'll start working on some of the recent issues that have popped up once I have some time available.

Contributor

Aesylwinn commented Oct 4, 2017

The files components/esm/loadland.hpp and components/esm/loadltex.hpp are good places to start. I believe there is a small discussion about LTEX records in the forum post linked in the opening post. You probably also want to look at apps/opencs/model/world/columnimp.hpp for the way to access and set the relevant data. Looking at how this PR "imports" LTEX records would probably also be beneficial. (files are off the top of my head, hopefully they are correct).

Seeing how PlutonicOverkill wants to continue, I'll start working on some of the recent issues that have popped up once I have some time available.

@Aesylwinn Aesylwinn deleted the Aesylwinn:landrecords branch May 5, 2018

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