Atahualpa's coding adventures

Involved development of the OpenMW construction set.
User avatar
Atahualpa
Posts: 1176
Joined: 09 Feb 2016, 20:03

Atahualpa's coding adventures

Post by Atahualpa »

Hey guys,

I've been inactive for the last weeks due to RL reasons -- and this won't change a lot in the next 4 to 5 weeks. While the work on additional videos is put on hold for the time being, I decided to spend my scattered free time digging through the OpenMW-CS source code. The reason is a very simple one: future moddability and OpenMW-CS rule! However, with aesylwinn retiring and Zini working on Titans of Ether, progress on our editor has stopped -- and I don't want to see a 0.42 release without any new OpenMW-CS features (okay, we have book texts now).

To cut a long story short: I have started to do a bit of coding. Please keep in mind that my coding abilities are just above the "can write spaghetti code" level. Currently, I'm focussing on table contents, layout, and value ranges for input fields (and corresponding verifier adjustments).

The following small "features" and fixes are ready for a PR:
  • [Done] Fix Book records to show Skill IDs instead of Attribute IDs for the teached skill.
  • [Done] Change the "Scroll" check box for Book records to a combo box (more user-friendly IMO).
  • [Done] Merge the "Skeleton Blood (White)", "Metal Blood (Golden)", and "Default (Red)" blood types for Creature and NPC records into a single combo box. (They can only have one type anyway.)
  • Merge the different light emitter types ("Flickering", "Slow Flickering", ...) for Light records into a single combo box. (They can only have one type too.)
  • Change the "Female" check box for NPC and Body Part records to a combo box (again, more user-friendly).
  • Some cosmetics: fixed typos, added comments to the new/adjusted code.
Of course, I will create new issues on our bug tracker or update existing ones.

WIP issues:
  • Integrate the "Blocked" and "References Persist" flags.
  • Make ranks editable in Faction records. (AFAIK, this isn't possible yet.)
  • Show actual rank names instead of integers in NPC records.
Future issues:
  • Add a "Reference Count" label to records in Objects table.
  • Calculate and display encumbrance for Container, Creature, and NPC records.
  • Re-arrange record layouts (especially to get rid of the space-consuming "Buys xyz" list in Creature and NPC records). I have already made some concepts for the different record types in the Objects table in Qt Designer.
  • Directly limit input in data fields (e.g., 0 ... 100 for disposition).
  • Adjust and complete the verifier functions.
  • Add detailed tool tips to all entries (with description, limits, enabled/disabled conditions, ...).
  • (Implement auto-calculation for NPC records.)
I'll tinker around with GitHub the next days. But I already have some questions regarding our code and PR conventions:

1. My first changes aren't very complex. Should I submit a pull request for every single of them anyway?
2. What is the "Faction ID" entry in NPC records for?
3. Should we limit data input to the original limitations present in TES CS? E.g., "Weapon Speed" and "Weapon Reach" values are limited to [0.10 ... 10].
4. How do you feel about fixing typos in our source code? E.g., I have renamed the member variable "mSkillID" to "mSkillId" to fit our naming conventions. There are also several misspelled instances of "referenceables"...
Last edited by Atahualpa on 16 Feb 2017, 21:48, edited 1 time in total.
User avatar
DestinedToDie
Posts: 1181
Joined: 29 Jun 2015, 09:08

Re: Atahualpa's coding adventures

Post by DestinedToDie »

Very nice.

I think the highest value target for OpenMW-CS is doormarkers. In CS an arrow is tied to a door. If you activate the door, it takes you to the position where the arrow is in. If you need a 3D arrow, let me know. It's easy to make. Or maybe make it so that the modder can assign their own 3D model, just like for normal objects?
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Atahualpa's coding adventures

Post by Zini »

1. My first changes aren't very complex. Should I submit a pull request for every single of them anyway?
Yes, please. Avoid large pull requests that are just a collection of various unrelated stuff. These are awful.
3. Should we limit data input to the original limitations present in TES CS? E.g., "Weapon Speed" and "Weapon Reach" values are limited to [0.10 ... 10].
Is there any value to that? Do values outside of this range cause trouble in OpenMW?
4. How do you feel about fixing typos in our source code? E.g., I have renamed the member variable "mSkillID" to "mSkillId" to fit our naming conventions. There are also several misspelled instances of "referenceables"...
Go ahead for spelling mistakes. Fixing naming conventions is a waste of time IMO.
User avatar
Atahualpa
Posts: 1176
Joined: 09 Feb 2016, 20:03

Re: Atahualpa's coding adventures

Post by Atahualpa »

Zini wrote:
3. Should we limit data input to the original limitations present in TES CS? E.g., "Weapon Speed" and "Weapon Reach" values are limited to [0.10 ... 10].
Is there any value to that? Do values outside of this range cause trouble in OpenMW?
I haven't tested this yet. So far, I created a list with all limitations of the original CS; the next step is to verify the severity of these limitations in both OpenMW and the vanilla engine. Restrictions like the maximum of 4 destinations per travel guide or 8 effects per potion may be related to the esm/esp format. We will see.
Allofich
Posts: 104
Joined: 28 May 2016, 12:50

Re: Atahualpa's coding adventures

Post by Allofich »

I just saw this thread now. Welcome to the coding side of the project!
User avatar
Atahualpa
Posts: 1176
Joined: 09 Feb 2016, 20:03

Re: Atahualpa's coding adventures

Post by Atahualpa »

Thanks, Allofich. :)


I've just submitted my first pull request with a fix for Bug #3746: OpenMW-CS: Book records show attribute IDs instead of skill IDs for teached skills entry.

Please let me know, if I did something wrong. (I've never worked with GitHub or something similar before.)
User avatar
Atahualpa
Posts: 1176
Joined: 09 Feb 2016, 20:03

Re: Atahualpa's coding adventures

Post by Atahualpa »

Okay, I have some more questions:
Atahualpa wrote: 2. What is the "Faction ID" entry in NPC records for?
5. Morrowind makes heavy use of flags. However, some flags (e.g., the two/three blood types) exclude each other. Should we check these flags for errors (more than one blood type at the same time)? If yes, where would we want to catch them? During the load process (error check in loadnpc.cpp)?

6. How do you feel about extensive comments in important but short files? E.g., I started to put comments in the various "loadxyz.hpp" files (in my experimental build). Have a look at the loadbook.hpp:

Code: Select all

#ifndef OPENMW_ESM_BOOK_H
#define OPENMW_ESM_BOOK_H

#include <string>

namespace ESM
{
/*
 * Books, magic scrolls, notes and so on
 */

class ESMReader;
class ESMWriter;

struct Book
{
    static unsigned int sRecordId;
    /// Return a string descriptor for this record type. Currently used for debugging / error logs only.
    static std::string getRecordType() { return "Book"; }

    struct BKDTstruct
    {
        float mWeight; // The book's weight.
		int mValue; // The book's coin value.
		int mIsScroll; // The book's type. (0 = book, 1 = scroll)
		int mSkillId; // What skills does it teach? (0...26 = skill ID, -1 = <none>)
		int mEnchant; // The book's enchantment points.
    };

    BKDTstruct mData; // Subrecord BKDT (20 bytes) --- The book's weight, coin value, book type, teached skill, and enchantment points.
	std::string mName; // Subrecord FNAM --- The book's in-game name. An empty name makes it inaccessible!
	std::string mModel; // Subrecord MODL --- The book's model. Must not be empty!
	std::string mIcon; // Subrecord ITEX --- The book's inventory icon. Must not be empty!
	std::string mScript; // Subrecord SCRI --- The attached script. Optional.
	std::string mEnchant; // Subrecord ENAM --- The book's enchantment. Only scrolls (mIsScroll = true) can have one. Optional.
	std::string mText; // Subrecord TEXT --- The actual book text. Optional.
    std::string mId; // Subrecord NAME --- The book's ID. Not editable.

    void load(ESMReader &esm, bool &isDeleted);
    void save(ESMWriter &esm, bool isDeleted = false) const;

    void blank();
    ///< Set record to default state (does not touch the ID).
};
}
#endif
As you can see, I added comments for every property of a book record. This may help to understand the meaning of all member variables (even in other places when using IntelliSense or other tools).
User avatar
scrawl
Posts: 2152
Joined: 18 Feb 2012, 11:51

Re: Atahualpa's coding adventures

Post by scrawl »

That is a perfect example of what I consider intrusive and overly verbose comments, which are a waste of time, hard to keep up to date and potentially even harmful.
Subrecord BKDT (20 bytes)
Why do I care about the subrecord name (implementation detail)? If I do, why not just jump to CPP file to see? Why do I need to know it is 20 bytes?
The book's model. Must not be empty!
Must not be empty or what? The game will explode? No, it will simply cause that object to not be rendered in the world. However, that's an implementation detail of the game, affects all types of game objects and has nothing to do with books.
The book's inventory icon. Must not be empty!
See above.
// Subrecord NAME --- The book's ID. Not editable.
If at all, this should be documented at some higher level since many records use that same type of ID. However, I don't see any reason why changing the ID of a record shouldn't become possible in the CS at some point in the future (provided there is a warning that any references to it in other files may be lost).

Either way, the esm/loadxyz files are definitely not the place for this type of documentation, even if was warranted. These files are concerned with the esm format/layout and nothing else.
User avatar
Atahualpa
Posts: 1176
Joined: 09 Feb 2016, 20:03

Re: Atahualpa's coding adventures

Post by Atahualpa »

scrawl wrote:That is a perfect example of what I consider intrusive and overly verbose comments, which are a waste of time, hard to keep up to date and potentially even harmful. [...]
I somehow knew that this would be an experienced coder's reaction. :D (And I share most of your concerns in this regard -- I just wasn't sure.)
Anyway, I only added the comments there because I lacked another place to put them. I know that we have a Wiki page about that particular topic (.esx file structure) but -- correct me, if I'm wrong -- the right place would be our documentation.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Atahualpa's coding adventures

Post by Zini »

#2: Sorry, no idea. Dig into the OpenMW codebase?

#5: Probably not very useful. We don't have a good way of dealing with non-critical errors that far down in components/esm anyway. Any error checking would have to happen in the load process of OpenMW-CS, which sounds like a lot of work for very little benefit.
If anything, I would improve the editor in a way that it is compatible with OpenMW when dealing with multiple flags. With your changes the editor clears all flags when multiple flags are set. We could improve upon that by looking what OpenMW is doing (or even what vanilla Morrowind is doing) and then mimic that in the editor. But I don't consider that an important change.

#6: I agree with scrawl there. These comments aren't all that useful, create additional work and can even get in the way. Let's not do that.
Post Reply