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.

Sign up for GitHub

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

Jump to bottom

fix: template not parsed #59

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

fix: template not parsed #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions packages/tiled/lib/src/objects/tiled_object.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ part of tiled;
/// Can contain at most one: <properties>, <ellipse> (since 0.9),
/// <point> (since 1.1), <polygon>, <polyline>, <text> (since 1.0)
class TiledObject {
int id;
int? id;
String name;
String type;
int? gid;
Expand All @@ -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.

Text? text;
bool visible;

Expand All @@ -72,7 +72,7 @@ class TiledObject {
String get class_ => type;

TiledObject({
required this.id,
this.id,
this.name = '',
this.type = '',
this.gid,
Expand Down Expand Up @@ -105,7 +105,7 @@ class TiledObject {
final width = parser.getDouble('width', defaults: 0);
final rotation = parser.getDouble('rotation', defaults: 0);
final visible = parser.getBool('visible', defaults: true);
final id = parser.getInt('id');
final id = parser.getIntOrNull('id');
final gid = parser.getIntOrNull('gid');
final name = parser.getString('name', defaults: '');

Expand All @@ -126,7 +126,8 @@ class TiledObject {
(xml) => xml.getChildren('point').isNotEmpty,
);
final text = parser.getSingleChildOrNullAs('text', Text.parse);
final template = parser.getSingleChildOrNullAs('template', Template.parse);
final template =
parser.getExternalPropertyOrNull('template', Template.parse);
final properties = parser.getProperties();

final polygon = parsePointList(parser, 'polygon');
Expand Down
40 changes: 37 additions & 3 deletions packages/tiled/lib/src/parser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ class ParsingException implements Exception {

class XmlParser extends Parser {
final XmlElement element;
XmlParser(this.element);
final Future<TsxProvider> Function(String key)? tsxProviderFunction;

XmlParser(
this.element, {
this.tsxProviderFunction,
});

@override
String? getStringOrNull(String name, {String? defaults}) {
Expand All @@ -21,15 +26,15 @@ class XmlParser extends Parser {
return element.children
.whereType<XmlElement>()
.where((e) => e.name.local == name)
.map(XmlParser.new)
.map(_newChild)
.toList();
}

List<Parser> getChildrenWithNames(Set<String> names) {
return element.children
.whereType<XmlElement>()
.where((e) => names.contains(e.name.local))
.map(XmlParser.new)
.map(_newChild)
.toList();
}

Expand All @@ -40,6 +45,28 @@ class XmlParser extends Parser {
) {
return xml(this);
}

@override
Future<T?> getExternalPropertyOrNull<T>(
String name,
T Function(Parser) parser,
) async {
final fileName = getStringOrNull(name);
if (fileName == null) {
return null;
}
final tsxProvider = await tsxProviderFunction?.call(fileName);
final result = tsxProvider?.getCachedSource() ?? tsxProvider?.getSource('');
if (result == null) {
return null;
}
return parser(result);
}

XmlParser _newChild(XmlElement element) => XmlParser(
element,
tsxProviderFunction: tsxProviderFunction,
);
}

class JsonParser extends Parser {
Expand Down Expand Up @@ -255,4 +282,11 @@ abstract class Parser {
}
return result;
}

Future<T?> getExternalPropertyOrNull<T>(
String name,
T Function(Parser) parser,
) async {
return null;
}
}
11 changes: 9 additions & 2 deletions packages/tiled/lib/src/tile_map_parser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,19 @@ class TileMapParser {
///
/// Accepts an optional list of external TsxProviders for external tilesets
/// referenced in the map file.
static TiledMap parseTmx(String xml, {List<TsxProvider>? tsxList}) {
static TiledMap parseTmx(
String xml, {
List<TsxProvider>? tsxList,
Future<TsxProvider> Function(String key)? tsxProviderFunction,
Comment on lines +15 to +16
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?

}) {
final xmlElement = XmlDocument.parse(xml).rootElement;
if (xmlElement.name.local != 'map') {
throw 'XML is not in TMX format';
}
final parser = XmlParser(xmlElement);
final parser = XmlParser(
xmlElement,
tsxProviderFunction: tsxProviderFunction,
);
return TiledMap.parse(parser, tsxList: tsxList);
}
}
1 change: 1 addition & 0 deletions packages/tiled/lib/src/tiled_map.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ class TiledMap {
return TileMapParser.parseTmx(
contents,
tsxList: tsxProviders.isEmpty ? null : tsxProviders,
tsxProviderFunction: tsxProviderFunction,
);
}

Expand Down