-
Notifications
You must be signed in to change notification settings - Fork 121
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
GSoC project: Tiled compatibility support Enigma #2302
base: master
Are you sure you want to change the base?
GSoC project: Tiled compatibility support Enigma #2302
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2302 +/- ##
==========================================
+ Coverage 35.27% 35.54% +0.26%
==========================================
Files 213 215 +2
Lines 20474 20498 +24
==========================================
+ Hits 7223 7286 +63
+ Misses 13251 13212 -39 see 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Co-authored-by: Thorbjørn Lindeijer <bjorn@lindeijer.nl>
CommandLine/libEGM/tsx.cpp
Outdated
// PackRes(resMap, dir, ids, child, msg, depth + 1); | ||
break; | ||
} | ||
case CppType::CPPTYPE_INT32: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the same between the xml-based readers. Feel free to abstract this and use an include rather than copy-pasting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright will try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have to look over the actual TSX logic in good detail, but here's some initial comments. Looking good so far; thank you!
shared/protos/Room.proto
Outdated
@@ -86,17 +86,18 @@ message Room { | |||
optional string name = 1; | |||
optional int32 id = 2 [(id_start) = 10000001]; | |||
optional string background_name = 3 [(resource_ref)="background", (gmx) = "bgName"]; | |||
optional double x = 4; | |||
optional double y = 5; | |||
optional double x = 4 [(tmx) = "x"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this property when the tmx field has the same name? You'll notice we omit the gmx attributes when the name is he same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only loading the values of those fields which has tmx extension, rest are ignored. I also realized that number of fields not directly corresponding to tmx attributes >> number of fields which have direct link. So I decided to keep tmx for same name fields as well.
optional int32 vertical_spacing = 10 [(gmx) = "tilevsep"]; | ||
optional int32 tile_width = 5 [(gmx) = "tilewidth", (tmx) = "tilewidth"]; | ||
optional int32 tile_height = 6 [(gmx) = "tileheight", (tmx) = "tileheight"]; | ||
optional int32 horizontal_offset = 7 [(gmx) = "tilexoff", (tmx) = "margin"]; // tiled use single integer for both... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't seen the rest of the PR, yet, but are these annotations useful? Are we able to sort by tag number (in this case, 7 and 8) and just comma-separate, in all cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get that. Like sorting and comma-separate thing? Are you talking about proto fields extension, then it loads automatically. Here is loading logic https://github.com/KartikShrivastava/enigma-dev/blob/5614cfbd6d67e668d1170ce7a52f6ffb9f89d2f6/CommandLine/libEGM/General/tiled_util.cpp#L36
namespace enigma_user { | ||
|
||
bool is_base64(unsigned char c) { | ||
return (isalnum(c) || (c == '+') || (c == '/')); | ||
return ::is_base64(c); | ||
} | ||
|
||
string base64_encode(string const& str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the header, you can just put using ::base64_encode;
(etc) instead of declaring them as wrapper impls, here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did that but is not it less readable and more confusing?
@@ -0,0 +1,389 @@ | |||
#include "zlib_util.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did Robert write this? Or is this Zlib-licensed Zlib code? We should include a license header either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if its zlib-licensed code, I copied it from gmk so copied its license.
libpng_encode32_file(rgba.data(), w, h, write_fname.u8string().c_str()); | ||
} | ||
|
||
void writeTempBGRAFile(const std::filesystem::path& write_fname, std::unique_ptr<char[]> bytes, size_t width, size_t height) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this logic go? Was it unused...? It does look a little hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied it to zlib_util.cpp where its used, just didn't put it in Decoder class, as it wasn't there previously as well. Its static right now.
…le[Width[Height]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments based on browsing the latest changes.
CommandLine/libEGM/tmx.cpp
Outdated
tile->set_x(currX * (mapTileWidth - (hexMapUtil->hexSideLength / 2))); | ||
|
||
if((currX%2 == 0 && hexMapUtil->staggerIndex == "odd") || (currX%2 != 0 && hexMapUtil->staggerIndex == "even")) | ||
if((currX%2 == 0 && hexMapUtil->staggerUtil.staggerIndex == "odd") || | ||
(currX%2 != 0 && hexMapUtil->staggerUtil.staggerIndex == "even")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to do these comparisons a lot (like, for each tile), it could be a good idea to avoid doing string comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced string staggerIndex and staggerAxis with enum class members
CommandLine/libEGM/tmx.h
Outdated
/** | ||
* @brief Hex map is a staggered map | ||
*/ | ||
StaggerUtil staggerUtil; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be reasons to prefer aggregation, but in this case I would be inclined to derive HexMapUtil
from StaggerUtil
instead.
Regarding the naming, I'm a bit confused about the choice of "Util", since at least personally I expect a utility to do something rather than just store some data. Maybe "Info", "Parameters" or "Properties" would be more suitable (or shorter versions, if you prefer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly I'm pretty bad at naming, updated those structs with Info suffix and inherited StaggerInfo from HexMapInfo
Also use GMRoom in Gamemaker specific formats
e8e7edd
to
411f672
Compare
In A Nutshell - This project implements two importers: .tmx/.tsx file importers in RadialGM and .egm file importer in Tiled
My GSoC project spans into three PRs in three different projects of two separate organizations. The two organizations involved are Enigma-dev and Tiled. fundies and Josh are my mentors from Enigma-dev and bjorn is my mentor from Tiled.
Requirement: You have to use Arch linux OS specifically, as Enigma-dev's RadialGM only compiles in Arch linux at the time of writing these steps. Compiling RadialGM can be tough, so feel free to ping in enigma-dev discord server.
Steps to test the project:
Short demo video of the final outcome: https://www.youtube.com/watch?v=ZUJd5VhqQo8
Weekly project work logs: https://kartikshrivastava.github.io/
Completed list of tasks in GSoC coding period:
Tiled TMX and TSX importer support status in RadialGM and enigma-dev:
Enigma's EGM importer support status in Tiled:
Follow-up list of subtasks after GSoC: