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

Fixed proposed changes to Flare plugin #1781

Merged
merged 12 commits into from Oct 12, 2017
Merged

Conversation

dorkster
Copy link
Contributor

#1417 appears to have been untouched for the passed 9 months or so. I've fixed the merge conflicts, as well as making a few style changes based on how we format data for Flare. The output we want is background_color=R,G,B,A, where RGBA are 8-bit integers.

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Looks good, I had mostly just a few more comments on the coding style.

I hope you don't mind if I squash these commits into a single change when merging.

x = loc[0].toFloat()*map->tileWidth();
y = loc[1].toFloat()*map->tileHeight();
x = (loc[0].toFloat())*map->tileWidth();
y = (loc[1].toFloat())*map->tileHeight();
Copy link
Member

Choose a reason for hiding this comment

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

Please don't add superfluous parenthesis here. I'd rather have spaces around the * operator.

@@ -285,6 +300,8 @@ bool FlarePlugin::write(const Tiled::Map *map, const QString &fileName)
}

QTextStream out(file.device());
QColor backgroundColor;
backgroundColor = map->backgroundColor();
Copy link
Member

Choose a reason for hiding this comment

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

Please merge these into one line.

Properties::const_iterator it = tileLayer->properties().constBegin();
Properties::const_iterator it_end = tileLayer->properties().constEnd();
for (; it != it_end; ++it) {
if(it->userType() == filePathTypeId()){
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: should have space after if and before the {. Also below.

if(it->userType() == filePathTypeId()){
out << it.key() << "=" << mapDir.relativeFilePath(toExportValue(it.value()).toString()) << "\n";
}
else{
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: should be } else {. Also below.

@dorkster
Copy link
Contributor Author

I've made the requested fixes. And feel free to squash the commits.

out << it.key() << "=" << it.value().toString() << "\n";
QVariantMap propsMap = o->properties();
for (QVariantMap::const_iterator it = propsMap.constBegin(); it != propsMap.constEnd(); ++it) {
if(it->userType() == filePathTypeId()){
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be such a nitpick, but please also add the spaces here and adjust the else below as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries. Just pushed a fix.

out << it.key() << "=" << mapDir.relativeFilePath(toExportValue(it.value()).toString()) << "\n";
} else {
out << it.key() << "=" << toExportValue(it.value()).toString() << "\n";
}
Copy link
Member

Choose a reason for hiding this comment

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

On closer inspection, I believe this could be simplified to just:

out << it.key() << "=" << toExportValue(it.value(), mapDir).toString() << "\n";

The same goes for the writing of layer properties.

The version of toExportValue taking an additional map directory is new and will handle the file path case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. That syntax is even used on line 324. I fixed the two spots that needed it.

@bjorn bjorn merged commit 395ddb1 into mapeditor:master Oct 12, 2017
@bjorn
Copy link
Member

bjorn commented Oct 12, 2017

Thanks!

@dorkster dorkster deleted the flare_plugin branch October 17, 2017 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants