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

Initial support for WangSets #226

Merged
merged 9 commits into from
Aug 5, 2022
Merged

Initial support for WangSets #226

merged 9 commits into from
Aug 5, 2022

Conversation

josebaMdeL
Copy link
Contributor

@josebaMdeL josebaMdeL commented Jun 13, 2022 •
edited
Loading

Initial support for WangSets.

I have added it by reading the TMX Map Format documentation and following some examples, but note that:

  1. My code doesn't feet too well on the current crate organization (I will need some help with this).
  2. I don't really know how WangSets work or how are they used inside Tiled (I would like someone who knows them to test it).
  3. I don't really know what I'm doing in general (no way to fix this at the moment ;) )

@josebaMdeL josebaMdeL changed the title Current Initial support for WangSets Jun 13, 2022
@josebaMdeL josebaMdeL mentioned this pull request Jun 13, 2022
Closed
@bjorn
Copy link
Member

bjorn commented Jun 13, 2022

Just a small bit of feedback from me for now, I would suggest you turn your Wang set example into a test instead, since it demonstrates nothing new as an example, but it would be good to cover the loading of Wang sets with a test.

@josebaMdeL
Copy link
Contributor Author

Just a small bit of feedback from me for now, I would suggest you turn your Wang set example into a test instead, since it demonstrates nothing new as an example, but it would be good to cover the loading of Wang sets with a test.

Done!

aleokdev
aleokdev reviewed Jun 18, 2022
View reviewed changes
Copy link
Contributor

@aleokdev aleokdev 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'm going to need @bjorn to proofread this though as I'm not very experienced with wang sets.

bjorn
bjorn reviewed Jun 18, 2022
View reviewed changes
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.

Some comments, mostly about naming and documentation.

@josebaMdeL
Added InvalidWangIdEncoding(String) to the Error enum. Also make Wang…
ec3965f 
…Id return this Error kind when FromStr encoding fails. Renamed some variables to keep rust's naming consistency. Corrected some documentation comments.
@josebaMdeL
Copy link
Contributor Author

josebaMdeL commented Jun 21, 2022 •
edited
Loading

Updated with proposed changes. I just forget to change the WangSetType's doc comment, but it will be corrected for the next commit.

aleokdev
aleokdev reviewed Jun 21, 2022
View reviewed changes
bjorn
bjorn reviewed Jun 22, 2022
View reviewed changes
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.

A few more comments.

@josebaMdeL
Changed Error::InvalidWWangIdEncoding to hold the wrongly parsed Stri…
26af4bc 
…ng inside a named field. Minor documentation and varaible naming corrections. Removed TileId field from WangTile; it will be stored together within a HashMap.
aleokdev
aleokdev reviewed Jul 14, 2022
View reviewed changes
Copy link
Contributor

@aleokdev aleokdev left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This should be the final review, I'm OK with this being on the next 0.10 release.

josebaMdeL and others added 2 commits July 14, 2022 17:49
@josebaMdeL @aleokdev
Update src/tileset/wangset/wang_tile.rs
d88a995 
Co-authored-by: Alejandro Perea <alexpro820@gmail.com>
@josebaMdeL
More small corrections
96c3870
aleokdev
aleokdev previously approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@aleokdev aleokdev left a comment

Choose a reason for hiding this comment

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

LGTM.

@aleokdev aleokdev requested a review from bjorn July 18, 2022 20:28
bjorn
bjorn previously approved these changes Aug 4, 2022
View reviewed changes
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 fine to me as well, though I left one comment that I was wondering about.

@josebaMdeL josebaMdeL dismissed stale reviews from bjorn and aleokdev via f585cdc August 5, 2022 09:48
bjorn
bjorn previously approved these changes Aug 5, 2022
View reviewed changes
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 great, thanks a lot for your work on this!

@bjorn bjorn merged commit 1a10faf into mapeditor:current Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn bjorn bjorn approved these changes

@aleokdev aleokdev aleokdev left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@josebaMdeL @bjorn @aleokdev