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

Add support to Lua exporter for object templates #1901

Merged
merged 2 commits into from Sep 5, 2018

Conversation

Projects
None yet
2 participants
@Spice-King
Contributor

Spice-King commented Mar 4, 2018

Properties are copied from the template, then overwritten by the instances's properties before being written to the export.

Pros:

  • Backwards compatible with older tools/importers
  • No need to check templates for properties
  • No extra runtime overhead, compared to not using templates

Cons:

  • Can't directly tell if the object had a template
  • Can't tell if any properties were from the template or not
Add support to Lua exporter for object templates
Properties are copied from the template, then merged with the instances's
properties before being written to the export.
@bjorn

This is a useful change, though as noted in issue #1850 I think it should be optional.

Unfortunately, no export options exist at the moment so a bunch of code will have to be written to enable this. I imagine it could be done as follows:

  • Add this option to the Preferences.
  • Add a nested struct Options { bool detachTemplates = false }; to the MapFormat class, and add it as a parameter to the write virtual function (of course, all overrides will need to be adjusted, but at least it won't have to be done again when adding additional options).
  • When exporting a map, read the option from the preferences and pass it to the write function using the Options struct.
  • Pass this option on to the LuaWriter and use as appropriate.
writeProperties(writer, mapObject->properties());
Properties props;
if (const ObjectTemplate *objectTemplate = mapObject->objectTemplate()) {
props.merge(objectTemplate->object()->properties());

This comment has been minimized.

@bjorn

bjorn Mar 6, 2018

Owner

Here, we could just assign instead of calling merge, given that props is still empty anyway.

Please also remove the brackets, since there is only a single statement.

@Spice-King

This comment has been minimized.

Contributor

Spice-King commented Mar 7, 2018

Funny thing, I was a good chunk of the way into doing something close to export options. One of the things I did want to include was CLI overrides for the export command. Spent a chunk of time tossing around what would do the least damage and where to integrate export options.

I had not figured out where I wanted to store it, stuffing it into the map file seemed too granular, but sticking it in the global preferences felt kinda buried. A middle ground of a project or a workspace kind of thing feels more like where is should be, but we don't have that yet. Might tackle that as a project later.

Also, we get into a bit of a odd spot when an exporter does not react to an export option. Someone will get confused by that. No nice idea comes to mind to solve that. Can't assume that the plugin list is fixed, so I can't just label which options work with what formats. Might just wing it for now and figure it out later.

I'll give it some TLC over the next day and work on the export options.

@bjorn

This comment has been minimized.

Owner

bjorn commented Mar 7, 2018

Funny thing, I was a good chunk of the way into doing something close to export options. One of the things I did want to include was CLI overrides for the export command. Spent a chunk of time tossing around what would do the least damage and where to integrate export options.

I find it usually quite annoying that such a large amount of time in programming is spent weighing in questions about what approach to take (or even just how to name something), rather than actually getting something done. But such is the life of a programmer, or maybe mostly the life of a perfectionist programmer... Anyway, if you have other suggestions I'd love to hear them. :-)

I had not figured out where I wanted to store it, stuffing it into the map file seemed too granular, but sticking it in the global preferences felt kinda buried. A middle ground of a project or a workspace kind of thing feels more like where is should be, but we don't have that yet. Might tackle that as a project later.

Yes, there are already many things that would work better when stored in a project or workspace file. This is covered by issue #1665 and it's on the Roadmap for Tiled 1.3.

Also, we get into a bit of a odd spot when an exporter does not react to an export option. Someone will get confused by that. No nice idea comes to mind to solve that. Can't assume that the plugin list is fixed, so I can't just label which options work with what formats. Might just wing it for now and figure it out later.

Actually all plugins are part of Tiled so we can make sure the setting is recognized in all places where the user would expect it.

Of course, there will probably also be a use-case for format-specific options. I have currently no design for this though, especially regarding how the UI would present such options to the user. I imagine a plugin would need to register or expose its options somewhere, such that a UI can be built for it dynamically. But this exporting option is pretty universal, as would be the option to embed tilesets, which can be added in the same way.

I'll give it some TLC over the next day and work on the export options.

Looking forward to it!

@bjorn

This comment has been minimized.

Owner

bjorn commented Sep 4, 2018

So, I ended up implementing the "Detach Templates" export option independently of the export format (fe13589), so that it is immediately supported by all formats without needing to modify each of them.

I think that obsoletes this change, though I'll keep the PR open for now as a reminder that the Lua format should still be fixed to at least refer to the template files if templates are not detached on export.

Tweaked the code a little
* Don't crash when saving a map with a template that failed to load
  (would have an ObjectTemplate, but without an object).

* Only one call to 'merge'.

@bjorn bjorn merged commit 6c9aff8 into bjorn:master Sep 5, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bjorn

This comment has been minimized.

Owner

bjorn commented Sep 5, 2018

I think that obsoletes this change, though I'll keep the PR open for now as a reminder that the Lua format should still be fixed to at least refer to the template files if templates are not detached on export.

On second thought I decided to merge this change in addition to the recently added export options, because I realized that in case of the Lua plugin, templates were already written out in a detached form (with all attributes being simply included, regardless of whether the object overrides them), with the exception of the custom properties. I believe this exception was just an oversight.

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