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

tiled content pipeline bugfixes & new fields #841

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

KatDevsGames
Copy link
Contributor

added support for reading type field from TiledMapPropertyContent (Tiled lets you set it...)
added support for repeatX/repeatY fields from Tiled image layers property (renderer still does not support this yet)

value read bugfix @TiledMapProperties.cs:10 - this got introduced when you pulled my previous pr (sorry)

added support for reading type field & repeatX/repeatY from Tiled image layers (renderer still does not support this)
property value read bugfix Til;edMapProperties.cs:10
this is a Cast function in CollisionComponent that ICollisionActor objects can use (not unlike the one that rigidbodies get in unity)
@tigurx
Copy link
Contributor

tigurx commented Apr 10, 2024

Hi @KatDevsGames ,

Your timing is absolutely impeccable. The RepeatX and RepeatY for the image layer in tiled is actually something I started looking into last night when trying to implement a parallax image layer for my project. Do you have any idea on how to approach this in regard to the renderer?

Please let me know if I can be of any assistance. I am not exactly sure how to approach it, but wouldn't mind helping tackle that if it is something that proves difficult.

  • James

@Gandifil
Copy link
Collaborator

Hi @KatDevsGames and @tigurx !

I have watched PR.

  • TiledMap: all good. What does do RepeatX and RepeatY? Duplicating image N times horizontal and vertical? If yes, I don't think implementation of this is too big trouble.
  • Collision: thanks for Contains method, I forgot about this. But I don't understand Cast. I mean it must be on another logic layer. Collision is not physics (but they are linked of course). CollisionComponent does not have to move actors and does not know about "hit". May be, for you better to do component which moves actors and asks CollisionComponent about collisions. Other question, if you need to update collisions for sequence of object (not all, not one). Is it true?

In future, please, make PR separately by theme.

@tigurx
Copy link
Contributor

tigurx commented Apr 28, 2024

@Gandifil , For RepeatX & RepeatY -- Yes, it just repeats the image N times along the respected axis like you suggested.

2024-04-28.15-58-35.mp4

Copy link
Contributor

@kaltinril kaltinril left a comment

Choose a reason for hiding this comment

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

Looks like Cast is added but not yet used. I see the bug fix. I see you also added 4 properties (type, propertyType, repeatX, repeatY).

I noticed you removed a constructor for TiledMapPropertyValue that used to allow passing in a TiledMapProperties instance. Was this intended?

/// <param name="distance">Maximum distance over which to cast the Collider(s).</param>
public int Cast(IEnumerable<ICollisionActor> actors, Vector2 move, List<CollisionEventArgs> hitBuffer, float distance)
{
hitBuffer.Clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter that hitBuffer.Clear() isn't called in the 2nd Cast? What if someone calls the 2nd public method instead, then the hitBuffer won't get cleared right?

@@ -6,6 +6,7 @@
using Microsoft.Xna.Framework;
using MonoGame.Extended.Collisions.Layers;
using MonoGame.Extended.Collisions.QuadTree;
using Newtonsoft.Json.Linq;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is MonoGame.Extended trying to stay AOT compliant?

This article on MonoGame says that Newtonsoft json are not AOT compliant for console? Also that Linq should be avoided? Also, why Newtonsoft's Linq vs System.Linq?

https://docs.monogame.net/articles/preparing_for_consoles.html

Properties = new();
}

public TiledMapPropertyValue(TiledMapProperties properties)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this removed for a reason?

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

Successfully merging this pull request may close these issues.

4 participants