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

Feature #1219 #553

Merged
merged 149 commits into from Apr 21, 2015

Conversation

Projects
None yet
3 participants
@cc9cii
Copy link
Contributor

cc9cii commented Apr 9, 2015

Working:

  • Actor - inventory, spells, destinations, AI packages
  • Container - contents
  • Pathgrid - points, edges
  • Region - sounds
  • Faction - reaction map
  • Race/Birthsign - spells
  • Enchantment/Spell - effects
  • Potion - effects
  • Armor/Clothes - part refs
  • Items/creatures - levelled lists
  • dialogue subview display in non-table format - levelled creatures/items
  • incorporated enum delegates
  • TopicInfos result script shown in dialogue subview

Known issue:

  • AiWander idle is shown as a single integer

sirherrbatka added some commits May 25, 2014

Merge remote-tracking branch 'master/refs' into dialog-fix
Conflicts:
	CMakeLists.txt
	apps/opencs/CMakeLists.txt
	apps/opencs/view/world/dialoguesubview.cpp
Merge branch 'dialog-fix' into NonTableFields
TODO stop failing epicly with git
Merge branch 'NonTableFields' of https://github.com/sirherrbatka/openmw
into NonTableFields

I have no idea what I'm doing

Conflicts:
	apps/opencs/model/world/idtable.cpp
	apps/opencs/model/world/refidadapter.hpp
	apps/opencs/model/world/refidadapterimp.hpp
@cc9cii

This comment has been minimized.

Copy link
Contributor Author

cc9cii commented Apr 18, 2015

Done. Please review.

cc9cii added some commits Apr 18, 2015

Revert auto expansion of enums as it was interfering with row based o…
…perations. Fix default values of magic effect skill & attributes.
@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Apr 18, 2015

  1. The tables in the dialogue subview are too small (vertically) to be useful.
  2. The layout of the dialogue sub views is odd. We seem to have all the regular (old) fields in a section with an additional scrollbar (which shouldn't be there to begin with) and then all the new fields outside of it. This separation does not make sense.
  3. Segfault on shutdown. Steps to reproduce:
    1. Edit Record for NPC referenceable
    2. Scroll down to destinations
    3. Add row
  4. Info record dialogue subviews are still missing the script field.
  5. Does not compile warning-free:

In file included from /home/marc/OpenMW/openmw/apps/opencs/model/world/refidcollection.cpp:9:0:
/home/marc/OpenMW/openmw/apps/opencs/model/world/refidadapterimp.hpp: In member function ‘QVariant CSMWorld::BodyPartRefIdAdapter::getNestedData(const CSMWorld::RefIdColumn_, const CSMWorld::RefIdData&, int, int, int) const [with ESXRecordT = ESM::Clothing]’:
/home/marc/OpenMW/openmw/apps/opencs/model/world/refidcollection.cpp:857:1: instantiated from here
/home/marc/OpenMW/openmw/apps/opencs/model/world/refidadapterimp.hpp:1734:21: warning: comparison is always true due to limited range of data type [-Wtype-limits]
/home/marc/OpenMW/openmw/apps/opencs/model/world/refidadapterimp.hpp: In member function ‘QVariant CSMWorld::BodyPartRefIdAdapter::getNestedData(const CSMWorld::RefIdColumn_, const CSMWorld::RefIdData&, int, int, int) const [with ESXRecordT = ESM::Armor]’:
/home/marc/OpenMW/openmw/apps/opencs/model/world/refidcollection.cpp:857:1: instantiated from here
/home/marc/OpenMW/openmw/apps/opencs/model/world/refidadapterimp.hpp:1734:21: warning: comparison is always true due to limited range of data type [-Wtype-limits]

@cc9cii

This comment has been minimized.

Copy link
Contributor Author

cc9cii commented Apr 18, 2015

Thanks for the review.

Points 1 and 2 are UI related and will always be subjective. I can go back to the previous way of having a single scrollbar (I found that having to scroll down all the time was getting irritating after a while).

Point 3: I can't reproduce the segfault at exit on my system. Any chance of a stack trace please?

Point 4: are you talking about ESM::DialInfo::mResultScript? This goes to the "Topic Infos" table, yes?

Point 5: I'll fix the warnings.

@cc9cii

This comment has been minimized.

Copy link
Contributor Author

cc9cii commented Apr 18, 2015

That warning appears to be gcc pedantry (see http://stackoverflow.com/questions/10434640/gcc-comparison-is-always-true-due-to-limited-range-of-data-type-in-template) but I'll still see if there is a workaround.

@cc9cii

This comment has been minimized.

Copy link
Contributor Author

cc9cii commented Apr 19, 2015

DialInfo::mResultScript

All done except for point 3 which I can't reproduce (even tried on my ubuntu vm).

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Apr 19, 2015

Another crash:

openmw-cs: /home/marc/OpenMW/openmw/apps/opencs/model/world/nestedtableproxymodel.cpp:62: virtual int CSMWorld::NestedTableProxyModel::rowCount(const QModelIndex&) const: Assertion `!index.isValid()' failed.

Happened after clicking on the Edit Record menu item of a NPC in the referenceable table.

Here is the callstack: https://gist.github.com/zinnschlag/b7e60b358ca17a783ce5

I would give you the callstack for the other segfault too, but I can't even get to it now.

@cc9cii

This comment has been minimized.

Copy link
Contributor Author

cc9cii commented Apr 19, 2015

Sorry, it was a copy/paste from an older branch that caused it. RelWithDebInfo has NDEBUG defined so I never saw the assert.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Apr 20, 2015

Okay. No problem. Shit happens.

Here is the other callstack: https://gist.github.com/zinnschlag/1f12a1a56b97de847620

@cc9cii

This comment has been minimized.

Copy link
Contributor Author

cc9cii commented Apr 21, 2015

It's a stab in the dark, but would you mind trying this one please.

@cc9cii

This comment has been minimized.

Copy link
Contributor Author

cc9cii commented Apr 21, 2015

I think the issue with undo() is existing in master branch already. undo() is using a pointer to a table model that is already deleted when a subview (or dialoguesubview) is closed. Not sure why the pointer is invalid at this point, since we get it from mData ??

I think one solution might be for the document to maintain the model pointers. Another might be to ask for accept/reject when a user tries to close a subview. Yet another might be using shared ptr.

How to reproduce:

open the referenceables table
modify a few things
close the subview
open a new subview (e.g. factions table)
undo <- crash

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Apr 21, 2015

Your fix seems to work. Will merge.

I think the issue with undo() is existing in master branch already. undo() is using a pointer to a table model that is already deleted when a subview (or dialoguesubview) is closed. Not sure why the pointer is invalid at this point, since we get it from mData ??

This is a separate issue and a seriously weird one. The models do not get deleted until the document is getting deleted. The logic here is correct. There must be a bug somewhere. Mind filing an issue on the tracker?

@zinnschlag zinnschlag merged commit f326b14 into OpenMW:master Apr 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Apr 21, 2015

Just a wild guess. Maybe the commands operate on the proxy models instead of the actual models. That would be wrong and result in the crash we are seeing because the proxy models will get deleted when the respective table view gets killed.

@cc9cii

This comment has been minimized.

Copy link
Contributor Author

cc9cii commented Apr 21, 2015

Your guess is correct. When CSVWorld::Table calls setModel() it passes the pointer of the proxy model. Qt then uses this when calling undo() triggered from signals.

@cc9cii cc9cii deleted the cc9cii:NonTableFields branch Apr 21, 2015

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