Navigation Menu

Skip to content
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

AURORA: GFF3Writer #300

Closed
wants to merge 3 commits into from
Closed

Conversation

Nostritius
Copy link
Contributor

I thought long about my first attempt on a gff3 writer, but ultimately decided that it would be better to write an external class for exporting gff3, since the internal structure of the GFF3File class makes it very difficult to modify values. Also it is difficult to get the necessary information for exporting in an easy way. This class is only for generating a gff3 tree and export it. I have made many unit tests and also tested it against the original kotor executable by replacing a simple file by one created by the writer, and it behaved as i expected.

@DrMcCoy
Copy link
Member

DrMcCoy commented Jun 25, 2018

@Nostritius
Copy link
Contributor Author

The memory leaks should now be fixed.

} else {
// If the values are complex (greater then 4 bytes) write the index to the field data.
stream.writeUint32LE(fieldDataIndex);
switch (field.type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that a repeat of lines 61ff?

Move that into a method getFieldSize() or something like that.


// Write labels.
for (size_t i = 0; i < _labels.size(); ++i) {
Common::UString label = _labels[i];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that copy there necessary?

namespace Aurora {

GFF3Writer::GFF3Writer(uint32 id, uint32 version) : _id(id), _version(version) {
_structs.push_back(GFF3WriterStructPtr(new GFF3WriterStruct(this)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use boost::make_shared instead of new'ing the struct into the shared_ptr (here and everywhere else as well)?

I.e. boost::make_shared<GFF3WriterStruct>(this) should be equivalent to boost::shared_ptr<GFF3WriterStruct>(new GFF3WriterStruct(this)), and gets rid of the explicit new.

* Determine if this field has simple values (less equal 32 bit) which are written in the field
* or complex values, bigger than 32bit like strings written in the field data section.
*/
bool simple =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a const.

break;
case GFF3Struct::kFieldTypeResRef:
stream.writeByte(MIN(static_cast<byte>(16), static_cast<byte>(field.stringValue.size())));
stream.write(field.stringValue.c_str(), field.stringValue.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still might write a longer string than 16 bytes here

field.type = GFF3Struct::kFieldTypeByte;
field.labelIndex = _parent->addLabel(label);
field.uint32Value = value;
_parent->_fields.push_back(field);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of code duplication here in these methods. Creating the field index and adding the field, i.e. everything except setting the actual value, can be abstracted into a method.

@Nostritius
Copy link
Contributor Author

The issues should now be fixed

/** Write the LocString to a write stream. */
void writeLocString(Common::WriteStream &stream, bool withNullTerminate = false);
void writeLocString(Common::WriteStream &stream, bool withNullTerminate = false) const ;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's now an extra space between the const and the ;


// Insert the struct to the field vector.
_parent->_fields.push_back(field);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace

void GFF3WriterStruct::addByte(const Common::UString &label, byte value) {
size_t index = _parent->createField(GFF3Struct::kFieldTypeByte, label);
_parent->_fields[index].uint32Value = value;
_fieldIndices.push_back(index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go even further. GFF3Writer::Field &GFF3WriterStruct::createField(GFF3Struct::FieldType type, const Common::UString &label). Calls _parent->createField(), put the index into _fieldIndices and returns a direct reference to the field.

Then you only need to do

createField(GFF3Struct::kFieldTypeByte, label).uint32Value = value;

etc. in each method.


class GFF3Writer {
public:
//TODO: Add a constructor consuming a GFF3File object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TODO:

/** Get the toplevel struct. */
GFF3WriterStructPtr getTopLevelStruct();

/** Write the gff3 to stream. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, capitalize GFF and GFF3.

stringValue = field.stringValue;
locStringValue = field.locStringValue;

if (field.voidData.get() != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should still do an unconditional field.voidData.reset() before that. Otherwise, "a = b", with a.voidData filled and b.voidData empty will not empty a.voidData.

}

// Write fields.
unsigned int fieldDataIndex = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the move to unsigned int instead of size_t here?

}
}

uint32 GFF3Writer::getFieldDataSize(const Aurora::GFF3Writer::Field &field) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can even be a static function in this file's scope. It doesn't need to be a method of GFF3Writer.

@Nostritius
Copy link
Contributor Author

The issues should now be fixed

@DrMcCoy DrMcCoy added this to File format writers in TODO: Standalone Jul 9, 2018
@DrMcCoy
Copy link
Member

DrMcCoy commented Jul 26, 2018

Merged as 4971a26...da35c58, thanks! :)

@DrMcCoy DrMcCoy closed this Jul 26, 2018
@Nostritius Nostritius deleted the aurora_gff3writer branch December 8, 2018 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
TODO: Standalone
File format writers
Development

Successfully merging this pull request may close these issues.

None yet

2 participants