karai17 / Simple-Tiled-Implementation

Tiled library for LÖVE
Other
840 stars 121 forks source link

Object properties are ignored in bump.lua plugin #133

Closed Nixola closed 7 years ago

Nixola commented 7 years ago

When the Bump plugin creates a Bump item it only gives it the tile's properties; even when one (or more) colbox is present, the colbox' properties are ignored. I can't think of a proper solution to this though; one has to ignore one of the tables, provide two tables or merge them, and each approach has drawbacks.

karai17 commented 7 years ago

I don't quite understand what you mean by this. Do you mean the object group inside of a tile (in Tiled, this would be 'collision' data)?

Nixola commented 7 years ago

Yes, I mean the "collision" data; the properties of the objects inside the object group inside of a tile are ignored, and the Bump item only has the tile data itself.

bjorn commented 7 years ago

It's because of the line t.properties = tile.properties, where t is then added to the world. This means that only the properties of the tile are available to the physics library and object.properties are ignored.

I guess that a for loop is missing that merges object.properties into t.properties before it is added to the world, so that global properties can be set on the tile but properties can also be set on each collision shape, possibly overriding any global properties.

Edit: To avoid modifying the tile properties, of course t.properties should be a new table, that both tile.properties and object.properties are merged into.

karai17 commented 7 years ago

I'm unsure how to deal with this, honestly. Sub-tile collision boxes always seemed weird to me, but I do want to support them as best I can. The problem with merging would be that it would not let you set individual properties for each object or the tile, it would be all or nothing. At that point, why not just put the properties in the tile to begin with?

As much as I appreciate Tiled having some sort of collision interface, I don't really know if I agree with the execution. There is obviously some use for adding objects into a tile such as if the tile is a slope, you can put a triangle in there for your collision system to pick up... But it makes things very complicated.

I think it may be more of an issue with how this is being used. bump.lua is an AABB collision system so it only accepts rectangles. You can't have things like slopes. Instead you could have half-tile collisions or whatever, but is that really useful? If you are building a map for instance, maybe it would be better to create the collision boxes on top of the finished map instead of having individual collision boxes for each tile. That would be more efficient for the collision system to deal with, too. A major concern with having lots of tiles is that floating points can cause players to get stuck on borders that are seemingly joined. This is especially true with box2d.

I'm open to suggestions on how this should be dealt with, because I just don't know myself.

bjorn commented 7 years ago

The problem with merging would be that it would not let you set individual properties for each object or the tile, it would be all or nothing. At that point, why not just put the properties in the tile to begin with?

This is not about the properties on a tile object, but rather about the properties on the collision shape added to the tile. They should override any collision properties set on the tile itself.

You're absolutely right that it makes sense to merge the collision shapes, but with the right libraries this can be automated. The upside is that manual construction of a collision layer is not necessary. A good example of a library doing this is Tiled2Unity. But rather than implementing something like that, it is perfectly fine to include a warning and pointing to the more optimal approach. But I think for freely placed colliding objects, bump.lua will perform good even with thousands of objects.

Btw, now that I read a bit about bump.lua, it actually does not use this properties field at all, right? So rather than doing any strange merging thing, what should really just happen is something like:

t.tile = tile
t.colBox = object

This way, when t comes back from bump.lua it is easy to access either t.tile.properties or t.colBox.properties depending on where somebody put his custom properties.

karai17 commented 7 years ago

But which object gets put into colBox?

Bobbyjoness commented 7 years ago

That looks like the best solution imo

On Jan 19, 2017 4:02 PM, "Thorbjørn Lindeijer" notifications@github.com wrote:

The problem with merging would be that it would not let you set individual properties for each object or the tile, it would be all or nothing. At that point, why not just put the properties in the tile to begin with?

This is not about the properties on the tile object, but rather about the properties on the collision shape added to the tile. They should override any collision properties set on the tile itself.

You're absolutely right that it makes sense to merge the collision shapes, but with the right libraries this can be automated. The upside is that manual construction of a collision layer is not necessary. A good example of a library doing this is Tiled2Unity http://www.seanba.com/Tiled2Unity. But rather than implementing something like that, it is perfectly fine to include a warning and pointing to the more optimal approach. But I think for freely placed colliding objects, bump.lua will perform good even with thousands or objects.

Btw, now that I read a bit about bump.lua, it actually does not use this properties field at all, right? So rather than doing any strange merging thing, what should really just happen is something like:

t.tile = tile t.colBox = object

This way, when t comes back from bump.lua it is easy to access either t.tile.properties or t.colBox.properties depending on where somebody put his custom properties.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/karai17/Simple-Tiled-Implementation/issues/133#issuecomment-273897599, or mute the thread https://github.com/notifications/unsubscribe-auth/AHyKHmPHvgB3FAby5yZgUZZI534Zeyrzks5rT89sgaJpZM4LlV09 .

bjorn commented 7 years ago

But which object gets put into colBox?

The object (a collision shape on a tile) for which a colBox is created.

I'm not sure btw, if the check on object.type == 'colBox' is really necessary here, given that these objects are usually added for the sole purpose of providing collision data.

It seems @DanielPower wrote this part of the code, so maybe he can weigh in on this issue.

karai17 commented 7 years ago

is colBox a new object type? STI is still based on Tiled 0.16, so right now collision objects are just normal shapes...

Nixola commented 7 years ago

You define the object type when you create one in Tiled. In order to have collision working, you need to define objects as of type "colBox" in order for the plugin[s?] to recognize it as collision data. Since I'm actually using objects to define other kind of data right now, I don't think the check should be removed. By the way, Bjorn's solution makes sense to me.

karai17 commented 7 years ago

I don't think that check is actually there...

Nixola commented 7 years ago

It is, it's in the plugin; I dunno how to link a file in a comment, but it's lines 31 and 64 in plugins/bump.lua.

bjorn commented 7 years ago

The 'colBox' checks are here:

https://github.com/karai17/Simple-Tiled-Implementation/blob/master/sti/plugins/bump.lua#L31 https://github.com/karai17/Simple-Tiled-Implementation/blob/master/sti/plugins/bump.lua#L64

Anyway those are fine. The problem is those lines:

https://github.com/karai17/Simple-Tiled-Implementation/blob/master/sti/plugins/bump.lua#L34 https://github.com/karai17/Simple-Tiled-Implementation/blob/master/sti/plugins/bump.lua#L67

Which I'd replace with the code I posted above.

karai17 commented 7 years ago

I'm just not sure what colBox is... as far as I am aware, the objects in a tile are just normal objects... rectangles, triangles, etc.. Where does colBox come into play?

Nixola commented 7 years ago

ColBox is the type of the object you want to define collisions with; if an object does not have that type (type is definable, it's a string) it will be ignored by the bump plugin.

karai17 commented 7 years ago

So it isn't something that Tiled implements, this is something @Bobbyjoness implemented?

Nixola commented 7 years ago

Basically, but it does only use Tiled features (an object type is defined within Tiled) and allows people to have object which do not represent collisions; I use them to represent spawn points and other events, too, kind of like RPG Maker XP

bjorn commented 7 years ago

So it isn't something that Tiled implements, this is something @Bobbyjoness implemented?

There's no @Bobbyjoness in the history of your bump.lua plugin. Like I said earlier, the code seems to be from @DanielPower.

Nixola commented 7 years ago

https://github.com/karai17/Simple-Tiled-Implementation/blob/master/sti/plugins/bump.lua#L3

bjorn commented 7 years ago

Right, sorry I just realized the history is cut because of a file rename (GitHub should handle that better...). Anyway, the code in question is still from @DanielPower, added in #129.

DanielPower commented 7 years ago

colBox is simply an arbitrary name check I added in case the user wanted to add an object for some other purpose than collision. It does not reference any STI feature.

I had not considered that someone might want to also import the colBox properties, since I had originally intended them to just be used for the collision box, and nothing else. I can add this tonight, unless someone beats me to it. It's a rather trivial addition.

@bjorn I would not merge the tile properties and object properties, as they may have purposefully different values for a variable of the same name, which would lead to overwriting intended values. Remember that tile properties are not only used for bump. The user can store all sorts of game specific information in there.

Edit: Just took a quick look at it again. As @bjorn said, I think we just need to add t.object = object at line 39. Will look more into it and make a pull request tonight.

karai17 commented 7 years ago

So what if the user wants multiple objects in the tile to be collidable?

karai17 commented 7 years ago

Okay so here is what I am going to do:

  1. I am going to change the box2d plugin and bump.lua plugin to be explicit when it comes to dealing with instanced tiles' objects. Currently the box2d plugin is implicit (it makes all objects within a tile collidable)
  2. I am going to change "colBox" to "collidable" in the bump.lua plugin to match the rest of STI's API
  3. I am going to add each object's properties to the processed object

@bjorn I think in Tiled, "collision" should be renamed since there is no actual collision happening in Tiled, and it is making an assumption that instances of objects in a tile are going to be collision objects.

bjorn commented 7 years ago

@karai17 You have to come up with a better name, because I don't know any. I'm afraid the user will no longer understand what the feature is for, when this term is replaced. You can't really call it the "Edit Tile Object Layer" dialog, right?

karai17 commented 7 years ago

Tiled is making an assumption that this is what the objects are to be used for. There are other potential uses for having instanced tile objects besides slamming into walls, such as sensors, spawn points, etc...

bjorn commented 7 years ago

@karai17 I know there are potential other uses (though "sensor" is part of the collision system really). The problem remains that other uses are rare and that I don't have a better generic name.

karai17 commented 7 years ago

this should be fixed now.