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

fix: template not parsed #59

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trunghq3101
Copy link

Description

Currently, template is parsed from a child node like this:

final template = parser.getSingleChildOrNullAs('template', Template.parse);

But according to the current Tiled format, template is not a child node anymore:

  <object id="31" template="card.tx" name="bank" gid="0" x="-256" y="736" rotation="0"/>

There is no place to load the template file as well.
So, it is never found, that's why the template field in a parsed TiledObject is always null. I believe it's related to the issue #47.

The object id is also not required anymore:

<?xml version="1.0" encoding="UTF-8"?>
<template>
 <object width="288" height="416"/>
</template>

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

Fixes #47

@spydon spydon requested review from kurtome and jtmcdole October 27, 2022 14:55
@@ -59,7 +59,7 @@ class TiledObject {
bool point;
bool rectangle;

Template? template;
Future<Template?>? template;
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be a future?

Copy link
Author

Choose a reason for hiding this comment

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

The template object must wait for the template file to be loaded.
We also can’t load it beforehand because we can’t know which file to load until reaching a template property.

Copy link
Member

Choose a reason for hiding this comment

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

But it will be loaded while parsing right? Since this is mutable for some reason it could just be set once it is finished?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it would be the easier way. However, all the call stack down to TiledObject.parse is synchronous, if we make it async, we must change the upstream too. I want to avoid a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

But since it is mutable you don't have to make it async, just set it when it is done?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, this new TiledObject instance is added to the final parsed object after load() function is completed. If the template is mutated after that, users can't know when it happens to get the latest value. If using Future, at least they know explicitly that they need to await it to get the value. That's my intention. But I have to admit that it's not the best solution.
I've just thought about another solution. If load() function somehow can await for all pending Futures complete, it's the best.

Copy link
Member

Choose a reason for hiding this comment

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

I've just thought about another solution. If load() function somehow can await for all pending Futures complete, it's the best.

Yeah, that sounds much better, that shouldn't be a problem to do.

@kurtome
Copy link
Collaborator

kurtome commented Oct 28, 2022

But according to the current Tiled format, template is not a child node anymore

Can you clarify what docs and when this changed? If it changed relatively recently we may want to check for the template xml the old way, and then if it's not there, check the new way. That would support both versions of Tiled. I would also add inline comments that link to the relevant Tiled docs and version changes

Comment on lines +15 to +16
List<TsxProvider>? tsxList,
Future<TsxProvider> Function(String key)? tsxProviderFunction,
Copy link
Member

Choose a reason for hiding this comment

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

what's the difference between these two? seems like two different ways to pass in external tsx files?

@spydon
Copy link
Member

spydon commented Nov 21, 2022

@trunghq3101 any update on this?

@trunghq3101
Copy link
Author

@trunghq3101 any update on this?

I'm gonna continue working on this. I'm kinda being distracted by working on my game lately.

@trunghq3101
Copy link
Author

But according to the current Tiled format, template is not a child node anymore

Can you clarify what docs and when this changed? If it changed relatively recently we may want to check for the template xml the old way, and then if it's not there, check the new way. That would support both versions of Tiled. I would also add inline comments that link to the relevant Tiled docs and version changes

According to this change log, template has been just a field of an object since day one. I didn't take a look at this when creating this PR. Is the any chance that our parser has never actually worked?

@spydon
Copy link
Member

spydon commented Oct 29, 2023

According to this change log, template has been just a field of an object since day one. I didn't take a look at this when creating this PR. Is the any chance that our parser has never actually worked?

Sorry for the extremely late reply, it should be pretty simple to verify that with a small test, right?

@spydon
Copy link
Member

spydon commented Dec 17, 2024

@trunghq3101 are you planning to continue on this PR? :)

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.

The Template cannot be properly parsed.
4 participants