[Bug] Follower not matching the Object

[kschoos]

After my fix on issue 345 I introduced a bug that shows the CompositeObject's BasicFollower at the wrong position, offset from where it belongs.

It now depends on how we want this to be fixed. The most straightforward way I can think of is setting the BasicFollower to encapsulate all the objects, including the origin of the compositeItem itself. This would handle rotation and scaling the best. I'll post a pic.

What do you think?

[azakhary]

hmm, I am bit in the middle of something, thus having trouble switching my mind to this. Please post pics, so it's easier to understand what's going on and why :))

[kschoos]

http://imgur.com/aRkHwUH

the origin of the CompositeItem is at [0,0] the coloured square is moved up and to the left inside it.

[azakhary]

wait this is on composite item? how does fix on #345 affecting this? the follower is using composite width and height, which are calculated with recalculate size method in composite system. Your fix did not touch things there, so I am not sure how this happens.

[kschoos]

width and height are the width and height of the boundingbox right ? They are calculated based on those afaik ?

[kschoos]

Yes, there we go. In CompositeSystem::recalculateSize we recalculated the width and the height of the object

[azakhary]

yeah.

here: https://github.com/UnderwaterApps/overlap2d-runtime-libgdx/blob/master/src/com/uwsoft/editor/renderer/systems/CompositeSystem.java#L120

we should have:

dimensionsComponent.boundBox.set(0, 0, dimensionsComponent.width, dimensionsComponent.height);

isn't this correct?

[azakhary]

I mean it was previously. but now I see what is going on :D ok let me think for a sec

[azakhary]

yeah this change is good, followers need to be changed so that they take this offset into account. agree.

[kschoos]

Okay. My fix is still okay right? The one I did in CompositeSystem?

[azakhary]

actually. I don't seem to have Adobe Flash at this moment. Is there any chance you have it? My rule is, when in doubt, see how Adobe did it.

[azakhary]

I think this might lead to problems. you always want to be able to see where the (0,0) point of a composite is. that is why the bounding was from 0,0 and that is why followers followed that rule

[azakhary]

if you want your things to be exactly in it, why not just offset the children to the needed position?

[kschoos]

Because that whats makes this not 0, 0 anymore, but rather the actual boundaries of the object. It was 0, 0 because of accident actually, you checked Math.Min() against 0, so that it would always show 0 when above 0 and always the value when below 0. I thought that must be an oversight.

[kschoos]

I was talking about my prior fix here /\

[kschoos]

I offset the children, thats what posed the problem in the first place

[azakhary]

Yeah I see now. It was unintended bug, but it lead to good user experience. Which means this is up to debate. I see the only problem the scenario where: you put things inside composite in very far coordinates, and then get out of it, and it looks really cool and all, but then your actual 0 point is in minus million or something.

[kschoos]

Hmm I dont see what you mean

[azakhary]

Previously it was a nice feature, because you could make composite "bigger" artificially by moving things to the top right, now you will not be able to do it, because it will be always the visual bound.

[kschoos]

You can still do that right? You move stuff inside the composite and the composite grows along

[kschoos]

But I think I understand what you mean.

[azakhary]

here is my suggestion. I will be touching this thing soon anyway, because I wanted to kill this all "recalculate" thing, and give people ability to play with composite size visually anyway. So at this moment, it's just a question to do this intuitive at this moment with current setup. Let's do the next best semi solution, that breaks least things. And then later i'll do my thing with "custom" sized composites.

[kschoos]

That's why I asked and didn't just jump in, because I feel like this little oversight, bug or whatever you want to call it in CompositeSystem leads to some problems down the road. So before I go into "fixing rampage" I wanna make sure we actually want to fix this ( I would :P )

[kschoos]

For example, when fixing this, I encountered a problem where, when you grab one handle of the Follower, the handle offsets itself by some amount before you can drag it. I think this is also due to the same thing.

[azakhary]

well we totally have to fix this now that followers are far from actual object and object does not look "inside" the follower :D here is what logic tells me:

Current temporary fix, just to make things ok. 1) items should always be inside the visual follower. 2) let's keep bounds to be from 0,0 to the right-top most point. so that when you move things right, composite grows. (follower becomes bigger, because of the bounds, which start at 0)

Future, more appropriate fix: 1) Composite Size-BOX, and composite Bounds-Box should be different things 2) Composite size is something that starts at 0 and wraps around items. 3) Follower represents composite size. 4) Composite size get's recalculated just once, per creation and then stays the same unless user changes it with special handles we make for it. 5) Composite Bounds is something else, it can have custom placement and custom size, relative to the composite, it is for collision reasons and things like that. 6) We later use composite-size box, for things like - align right, align top, margin e.g. 7) We use bounds-box for things like physics.

Does this make sense?

[kschoos]

Not completely... By 0,0, do we mean World(0, 0) or composite(0, 0)? What's the benefit of a fixed size composite box? // Got it, over read (6)

[azakhary]

composite 0,0. It's not fixed size, it's size is equal to it's childrens top-most point. (in this tmp fix)

[kschoos]

okay 1) and 2) is what I wanted to fix and showed you in the pictures. That can be done - however it leads to newly created bugs as stated above.

I don't really get the difference between composite size and composite bounds. Is it that composite bounds size does not rotate with the objects whereas composite bounds does ?

[kschoos]

I'm willing to fight though. FOR OVERLAP!

[azakhary]

I say - you do the fix you proposed anyway is it will be better then the current state. Do the PR, we'll look at it, Java is easier to understand then english. ^^ As soon as I get time to actually look at the code, I'll do it and go over it. I may be talking nonsense right now, as my thouhgts are elsewhere :)) When I look at it next time, things will be clearer.

[InnerScript]

I was just working on part of this, fixing the bounding and transform boxes when you edit polygon paths.

My thought was that the TranformComponent and DimensionsComponent should be updated to reflect the current size of the polygon object as it's edited.

My current progress: http://imgur.com/0HY4bdd

Next, I need to update the Transform/Dimensions, and fix the issue where growing the polygon in certain directions offsets the polygon x/y. I asked a question about where the drawables are rendered in a related issue. https://github.com/UnderwaterApps/overlap2d/issues/165#issuecomment-174226089

[kschoos]

Looks good InnerScript. Can't help you with your question though. I got another question: What is CompositeTransformComponent.transform and when is it set to true ?

I get a weird bug where my objects are not updated when I set their scale from something arbitrary to 1, 1.

Commenting out the conditionals in Overlap2DRenderer::drawRecursively and Overlap2DRenderer::drawChildren gives me a lot more sane behaviour.

I mean these conditionals: if (curCompositeTransformComponent.transform || transform.rotation != 0 || transform.scaleX !=1 || transform.scaleY !=1)

Why, when an object is not rotated or scaled it has to go through a giant process whereas when it is either rotated or scaled, it does not? Why are transforms only computed if the object was scaled or rotated, Translation is a transformation too, shouldn't it all be handled the same way? Scale something -> scale children recursively. Rotate something -> rotate children recursively. Translate something -> translate children recursively.

[azakhary]

this part is kind of an optimization. You see in order to do this complex transformation to entire composite a GPU call has to be made in order to change the matrix, so if we do it for each composite, this will become a very bad performance runtime. As a solution, because most of composites are neither rotated, nor scaled, we do this check. Mainly we check for rotation and scale. Although we also have this "user-defined" boolean just in case (which probably has no use)

[kschoos]

Got it! So I should rather use the boolean instead of fiddling with that code i guess ?

[azakhary]

No. What you mean by not getting updated? If your composite is neither rotated, nor scale. It should not go though this process. As there is nothing to transform.

[azakhary]

Maybe there is another way to do it. Just give more details on what the exact issue is.

[kschoos]

If I scale something to 1 / 1 from an arbitrary value, it's scale is set to 1 / 1 and therefor runs past the conditional

[azakhary]

runs past conditional and fails to do what exactly?

[kschoos]

Fails to update the visuals. It updates the DimensionComponent and TransformComponent, that I know because the Follower's size is changed. However the Sprite itself is not deformed correctly ( scaling ), it's not updated at all.

[kschoos]

For example, when I enter a deformed composite it does not update the composite's content's scaling. The Follower looks right but the sprite looks the same as it looks from outside the composite. It's hard to explain. Let's say you have

Composite [scale 0.5, scale 1.0] |> Composite [scale 0.5, scale 0.5] |> |> Sprite

Now at ROOT the thing should look like scale 0.25, 0.5 right? But when I enter Composite0, it still looks 0.25, 0.5 although it should look 0.5, 0.5. The follower looks 0.5, 0.5 thought.

Does that make any sense ?

[azakhary]

I see, that's weird, and new, maybe happened after changes @gevorg-kopalyan did lately.

[kschoos]

So can you reproduce it? It's not something I may have mistakenly done locally ?

[azakhary]

I have tried doing this on the version that can be downloaded from the website (last build) and was not able to reproduce. there it worked well. But I cannot test on the latest sources right now on this computer, till monday. So it's either something you did locally. Or related to latest changes on runtime. You can clone a fresh copy to test if it's you or the repo.

[kschoos]

Was able to reproduce with fresh sources. Also commenting out the conditionals fixes the error.

[azakhary]

So, that's totally something @gevorg-kopalyan should take a look at. Hopefully we get update about this tomorrow.

[kschoos]

Okay I'll not touch it then :)