Project

General

Profile

Bug #3066

OpenMW-CS: Editor doesn't check if IDs and other strings are longer than their hardcoded field length

Added by Who Knows almost 2 years ago. Updated 9 months ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
Editor
Target version:
Start date:
12/08/2015
% Done:

0%

Reproducibility:
Always
Operating system:
Other
Severity:
Normal

Description

from https://forum.openmw.org/viewtopic.php?f=2&t=3233

Script IDs can not be longer than 32 characters, because 
of the data structure used for saving (inherited from vanilla MW). 
https://github.com/OpenMW/openmw/blob/m ... pt.hpp#L34

I presume that OpenCS does not do any checking that created script IDs are 
actually within that character limit. Please raise a bug report.

script_not_found.png View (280 KB) Who Knows, 12/08/2015 03:34 AM

History

#2 Updated by Marc Zinnschlag almost 2 years ago

  • Subject changed from the CS doesn't check if the script ID is longer than 32 characters to CS doesn't check if IDs and other strings are longer than their hardcoded field length
  • Category set to Editor
  • Target version set to openmw-0.38

e.g. Script IDs, ranks names in a faction recrod, sound IDs and probably many others I missed. The code for this needs to go into the verifier.

#3 Updated by scrawl . almost 2 years ago

Wouldn't it be better to set the respective character limit on the input field, instead of checking in the verifier? Prevents the issue from happening in the first place rather than just warning about it.

#4 Updated by scrawl . almost 2 years ago

e.g. Script IDs, ranks names in a faction recrod, sound IDs and probably many others I missed

And the list of items in a container store. Effectively we have an ID length limit on almost all record types. We might as well apply the limit to all types then just for consistency's sake. Of course we can still lift the limitation post-1.0 when the file format is changed.

#5 Updated by cc 9cii almost 2 years ago

  • Operating system changed from Linux to Other

Apologies in advance for not submitting a pull request. Hopefully someone will be kind enough to help out. Below can be improved, but it is a start.

 apps/opencs/model/world/columnbase.cpp          |  3 ++-
 apps/opencs/model/world/columnbase.hpp          |  1 +
 apps/opencs/model/world/columnimp.hpp           |  3 ++-
 apps/opencs/model/world/data.cpp                |  4 ++--
 apps/opencs/model/world/refidcollection.cpp     |  9 ++++++---
 apps/opencs/view/world/cellcreator.cpp          |  7 +++++++
 apps/opencs/view/world/genericcreator.cpp       |  5 +++++
 apps/opencs/view/world/genericcreator.hpp       |  2 ++
 apps/opencs/view/world/idcompletiondelegate.cpp | 16 +++++++++++++++
 apps/opencs/view/world/referenceablecreator.cpp | 26 +++++++++++++++++++++++++
 apps/opencs/view/world/referenceablecreator.hpp |  3 +++
 apps/opencs/view/world/util.cpp                 | 10 +++++++++-
 12 files changed, 81 insertions(+), 8 deletions(-)

diff --git a/apps/opencs/model/world/columnbase.cpp b/apps/opencs/model/world/columnbase.cpp
index 1f16c96..a4fe98d 100644
--- a/apps/opencs/model/world/columnbase.cpp
+++ b/apps/opencs/model/world/columnbase.cpp
@@ -103,7 +103,8 @@ bool CSMWorld::ColumnBase::isId (Display display)
 bool CSMWorld::ColumnBase::isText (Display display)
 {
     return display==Display_String || display==Display_LongString ||
-        display==Display_String32 || display==Display_LongString256;
+        display==Display_String32 || display==Display_String64 ||
+        display==Display_LongString256;
 }

 bool CSMWorld::ColumnBase::isScript (Display display)
diff --git a/apps/opencs/model/world/columnbase.hpp b/apps/opencs/model/world/columnbase.hpp
index c75a3c2..dec9ee0 100644
--- a/apps/opencs/model/world/columnbase.hpp
+++ b/apps/opencs/model/world/columnbase.hpp
@@ -129,6 +129,7 @@ namespace CSMWorld
             Display_InfoCondVar,
             Display_InfoCondComp,
             Display_String32,
+            Display_String64,
             Display_LongString256,

             Display_EffectSkill,     // must display at least one, unlike Display_Skill
diff --git a/apps/opencs/model/world/columnimp.hpp b/apps/opencs/model/world/columnimp.hpp
index 4e608db..24414db 100644
--- a/apps/opencs/model/world/columnimp.hpp
+++ b/apps/opencs/model/world/columnimp.hpp
@@ -317,7 +317,8 @@ namespace CSMWorld
     template<typename ESXRecordT>
     struct NameColumn : public Column<ESXRecordT>
     {
-        NameColumn() : Column<ESXRecordT> (Columns::ColumnId_Name, ColumnBase::Display_String) {}
+        NameColumn(ColumnBase::Display display = ColumnBase::Display_String)
+            : Column<ESXRecordT> (Columns::ColumnId_Name, display) {}

         virtual QVariant get (const Record<ESXRecordT>& record) const
         {
diff --git a/apps/opencs/model/world/data.cpp b/apps/opencs/model/world/data.cpp
index 7214e7e..9361562 100644
--- a/apps/opencs/model/world/data.cpp
+++ b/apps/opencs/model/world/data.cpp
@@ -107,7 +107,7 @@ CSMWorld::Data::Data (ToUTF8::FromType encoding, const ResourcesManager& resourc
     mFactions.addColumn (new StringIdColumn<ESM::Faction>);
     mFactions.addColumn (new RecordStateColumn<ESM::Faction>);
     mFactions.addColumn (new FixedRecordTypeColumn<ESM::Faction> (UniversalId::Type_Faction));
-    mFactions.addColumn (new NameColumn<ESM::Faction>);
+    mFactions.addColumn (new NameColumn<ESM::Faction>(ColumnBase::Display_String32));
     mFactions.addColumn (new AttributesColumn<ESM::Faction> (0));
     mFactions.addColumn (new AttributesColumn<ESM::Faction> (1));
     mFactions.addColumn (new HiddenColumn<ESM::Faction>);
@@ -291,7 +291,7 @@ CSMWorld::Data::Data (ToUTF8::FromType encoding, const ResourcesManager& resourc
     mCells.addColumn (new StringIdColumn<Cell>);
     mCells.addColumn (new RecordStateColumn<Cell>);
     mCells.addColumn (new FixedRecordTypeColumn<Cell> (UniversalId::Type_Cell));
-    mCells.addColumn (new NameColumn<Cell>);
+    mCells.addColumn (new NameColumn<Cell>(ColumnBase::Display_String64));
     mCells.addColumn (new FlagColumn<Cell> (Columns::ColumnId_SleepForbidden, ESM::Cell::NoSleep));
     mCells.addColumn (new FlagColumn<Cell> (Columns::ColumnId_InteriorWater, ESM::Cell::HasWater,
         ColumnBase::Flag_Table | ColumnBase::Flag_Dialogue | ColumnBase::Flag_Dialogue_Refresh));
diff --git a/apps/opencs/model/world/refidcollection.cpp b/apps/opencs/model/world/refidcollection.cpp
index 7737e62..23a4088 100644
--- a/apps/opencs/model/world/refidcollection.cpp
+++ b/apps/opencs/model/world/refidcollection.cpp
@@ -58,7 +58,9 @@ CSMWorld::RefIdCollection::RefIdCollection(const CSMWorld::Data& data)

     NameColumns nameColumns (modelColumns);

-    mColumns.push_back (RefIdColumn (Columns::ColumnId_Name, ColumnBase::Display_String));
+    // Only items that can be placed in a container have the 32 character limit, but enforce
+    // that for all referenceable types for now.
+    mColumns.push_back (RefIdColumn (Columns::ColumnId_Name, ColumnBase::Display_String32));
     nameColumns.mName = &mColumns.back();
     mColumns.push_back (RefIdColumn (Columns::ColumnId_Script, ColumnBase::Display_Script));
     nameColumns.mScript = &mColumns.back();
@@ -239,9 +241,9 @@ CSMWorld::RefIdCollection::RefIdCollection(const CSMWorld::Data& data)
     mColumns.back().addColumn(
             new RefIdColumn (Columns::ColumnId_AiWanderRepeat, CSMWorld::ColumnBase::Display_Boolean));
     mColumns.back().addColumn(
-            new RefIdColumn (Columns::ColumnId_AiActivateName, CSMWorld::ColumnBase::Display_String));
+            new RefIdColumn (Columns::ColumnId_AiActivateName, CSMWorld::ColumnBase::Display_String32));
     mColumns.back().addColumn(
-            new RefIdColumn (Columns::ColumnId_AiTargetId, CSMWorld::ColumnBase::Display_String));
+            new RefIdColumn (Columns::ColumnId_AiTargetId, CSMWorld::ColumnBase::Display_String32));
     mColumns.back().addColumn(
             new RefIdColumn (Columns::ColumnId_AiTargetCell, CSMWorld::ColumnBase::Display_String));
     mColumns.back().addColumn(
@@ -486,6 +488,7 @@ CSMWorld::RefIdCollection::RefIdCollection(const CSMWorld::Data& data)
     mColumns.push_back (RefIdColumn (Columns::ColumnId_Class, ColumnBase::Display_Class));
     npcColumns.mClass = &mColumns.back();

+    // NAME32 enforced in IdCompletionDelegate::createEditor()
     mColumns.push_back (RefIdColumn (Columns::ColumnId_Faction, ColumnBase::Display_Faction));
     npcColumns.mFaction = &mColumns.back();

diff --git a/apps/opencs/view/world/cellcreator.cpp b/apps/opencs/view/world/cellcreator.cpp
index 2a710a9..58082f5 100644
--- a/apps/opencs/view/world/cellcreator.cpp
+++ b/apps/opencs/view/world/cellcreator.cpp
@@ -84,6 +84,13 @@ void CSVWorld::CellCreator::setType (int index)
     mYLabel->setVisible (index==1);
     mY->setVisible (index==1);

+    // The cell name is limited to 64 characters. (ESM::Header::GMDT::mCurrentCell)
+    std::string text = mType->currentText().toStdString();
+    if (text == "Interior Cell")
+        GenericCreator::setEditorMaxLength (64);
+    else
+        GenericCreator::setEditorMaxLength (32767);
+
     update();
 }

diff --git a/apps/opencs/view/world/genericcreator.cpp b/apps/opencs/view/world/genericcreator.cpp
index df77399..85f6f8a 100644
--- a/apps/opencs/view/world/genericcreator.cpp
+++ b/apps/opencs/view/world/genericcreator.cpp
@@ -174,6 +174,11 @@ CSVWorld::GenericCreator::GenericCreator (CSMWorld::Data& data, QUndoStack& undo
     connect (&mData, SIGNAL (idListChanged()), this, SLOT (dataIdListChanged()));
 }

+void CSVWorld::GenericCreator::setEditorMaxLength (int length)
+{
+    mId->setMaxLength (length);
+}
+
 void CSVWorld::GenericCreator::setEditLock (bool locked)
 {
     mLocked = locked;
diff --git a/apps/opencs/view/world/genericcreator.hpp b/apps/opencs/view/world/genericcreator.hpp
index f63c451..41f88e8 100644
--- a/apps/opencs/view/world/genericcreator.hpp
+++ b/apps/opencs/view/world/genericcreator.hpp
@@ -78,6 +78,8 @@ namespace CSVWorld

             std::string getNamespace() const;

+            void setEditorMaxLength(int length);
+
         private:

             void updateNamespace();
diff --git a/apps/opencs/view/world/idcompletiondelegate.cpp b/apps/opencs/view/world/idcompletiondelegate.cpp
index 9704908..d43d46f 100644
--- a/apps/opencs/view/world/idcompletiondelegate.cpp
+++ b/apps/opencs/view/world/idcompletiondelegate.cpp
@@ -30,6 +30,22 @@ QWidget *CSVWorld::IdCompletionDelegate::createEditor(QWidget *parent,
     CSMWorld::IdCompletionManager &completionManager = getDocument().getIdCompletionManager();
     CSVWidget::DropLineEdit *editor = new CSVWidget::DropLineEdit(display, parent);
     editor->setCompleter(completionManager.getCompleter(display).get());
+    // The savegame format limits the player faction string to 32 characters.
+    // The region sound name is limited to 32 characters. (ESM::Region::SoundRef::mSound)
+    // The script name is limited to 32 characters. (ESM::Script::SCHD::mName)
+    // The cell name is limited to 64 characters. (ESM::Header::GMDT::mCurrentCell)
+    if (display == CSMWorld::ColumnBase::Display_Faction ||
+        display == CSMWorld::ColumnBase::Display_Sound ||
+        display == CSMWorld::ColumnBase::Display_Script ||
+        display == CSMWorld::ColumnBase::Display_Referenceable)
+    {
+        editor->setMaxLength (32);
+    }
+    else if (display == CSMWorld::ColumnBase::Display_Cell)
+    {
+        editor->setMaxLength (64);
+    }
+
     return editor;
 }

diff --git a/apps/opencs/view/world/referenceablecreator.cpp b/apps/opencs/view/world/referenceablecreator.cpp
index 836e8ac..5de5f71 100644
--- a/apps/opencs/view/world/referenceablecreator.cpp
+++ b/apps/opencs/view/world/referenceablecreator.cpp
@@ -33,6 +33,8 @@ CSVWorld::ReferenceableCreator::ReferenceableCreator (CSMWorld::Data& data, QUnd
     }

     insertBeforeButtons (mType, false);
+
+    connect (mType, SIGNAL (currentIndexChanged (int)), this, SLOT (setType (int)));
 }

 void CSVWorld::ReferenceableCreator::reset()
@@ -53,3 +55,27 @@ void CSVWorld::ReferenceableCreator::toggleWidgets(bool active)
     CSVWorld::GenericCreator::toggleWidgets(active);
     mType->setEnabled(active);
 }
+
+void CSVWorld::ReferenceableCreator::setType (int index)
+{
+    // container items have name limit of 32 characters
+    std::string text = mType->currentText().toStdString();
+    if (text == "Potion" ||
+        text == "Apparatus" ||
+        text == "Armor" ||
+        text == "Book" ||
+        text == "Clothing" ||
+        text == "Ingredient" ||
+        text == "ItemLevelledList" ||
+        text == "Light" ||
+        text == "Lockpick" ||
+        text == "Miscellaneous" ||
+        text == "Probe" ||
+        text == "Repair" ||
+        text == "Weapon")
+    {
+        GenericCreator::setEditorMaxLength (32);
+    }
+    else
+        GenericCreator::setEditorMaxLength (32767);
+}
diff --git a/apps/opencs/view/world/referenceablecreator.hpp b/apps/opencs/view/world/referenceablecreator.hpp
index 14ad24b..57b5b03 100644
--- a/apps/opencs/view/world/referenceablecreator.hpp
+++ b/apps/opencs/view/world/referenceablecreator.hpp
@@ -29,6 +29,9 @@ namespace CSVWorld

             virtual void toggleWidgets(bool active = true);

+        private slots:
+
+            void setType (int index);
     };
 }

diff --git a/apps/opencs/view/world/util.cpp b/apps/opencs/view/world/util.cpp
index 1da9878..179f799 100644
--- a/apps/opencs/view/world/util.cpp
+++ b/apps/opencs/view/world/util.cpp
@@ -261,7 +261,15 @@ QWidget *CSVWorld::CommandDelegate::createEditor (QWidget *parent, const QStyleO
             widget->setMaxLength (32);
             return widget;
         }
-            
+
+        case CSMWorld::ColumnBase::Display_String64:
+        {
+        // For other Display types (that represent record IDs) with drop support IdCompletionDelegate is used
+            CSVWidget::DropLineEdit *widget = new CSVWidget::DropLineEdit(display, parent);
+            widget->setMaxLength (64);
+            return widget;
+        }
+
         default:

             return QStyledItemDelegate::createEditor (parent, option, index);

#6 Updated by Who Knows almost 2 years ago

Jannik Heller wrote:

We might as well apply the limit to all types then just for consistency's sake. Of course we can still lift the limitation post-1.0 when the file format is changed.

Or … make it better than TES3 and allow longer IDs?! What would harm here? If the esm IDs are always limited by the TES3-CS, why would allowing longer IDs in omwaddon-files be a problem? I can't imagine that the omwaddon format has to be rewritten, right? There are markers for or file pointers to the specific data plus a data size value, right?
AFAIK it will not be supported to make omwaddon files readable by the TES engine, right?

#7 Updated by scrawl . almost 2 years ago

The problem is that some fields are stored as fixed-size string in the file (e.g. a NAME32). There is no 'this string is 32 characters long' marker before that string is written, the format is simply hardwired to always expect the string to be 32 characters long.

In theory you can rename an .omwaddon to .esp and make it load in the original engine, though this hasn't been tested much. AFAIK (don't quote me on this) Zini's plan was to keep the vanilla format until 1.0, then start changing and expanding it post 1.0, which sounds like a good plan to me.

#8 Updated by Who Knows almost 2 years ago

Jannik Heller wrote:

The problem is that some fields are stored as fixed-size string in the file (e.g. a NAME32). There is no 'this string is 32 characters long' marker before that string is written, the format is simply hardwired to always expect the string to be 32 characters long.
[...]
Zini's plan was to keep the vanilla format until 1.0, then start changing and expanding it post 1.0, which sounds like a good plan to me.

Hm, I see. Having flexible string size would mean to change elemental characteristics in the omwaddon files¹. I support extending the omwaddon format for >1.0 then, too.

¹ Except for setting a default fixed string size of 2x32 or 4x32 characters. Admittedly this would break compatibility to the TES3 engine.

#9 Updated by Marc Zinnschlag over 1 year ago

  • Target version changed from openmw-0.38 to openmw-0.39

#10 Updated by Marc Zinnschlag over 1 year ago

  • Target version changed from openmw-0.39 to openmw-0.40

#11 Updated by Marc Zinnschlag about 1 year ago

  • Target version changed from openmw-0.40 to openmw-cs-1.0

#12 Updated by Jeffrey Haines 9 months ago

  • Subject changed from CS doesn't check if IDs and other strings are longer than their hardcoded field length to OpenMW-CS: Editor doesn't check if IDs and other strings are longer than their hardcoded field length

Unify all editor ticket subjects to start with "OpenMW-CS:"

Also available in: Atom PDF