Draw() bug when viewport is offset from map

[greghouston]

If the viewport is offset from the map, anything drawn in an object will be equally offset from that object.

Here the viewport is offset 32px to the right:
http://greghoustondesign.com/sandbox/game-issue/index.html

Note in the bottom right corner that the text and their objects are no longer aligned. The text is 32px to the right.

Scroll down to the bottom right of the page. Add a couple enemies. Note that their health bars are no longer centered above them.

Click on the blue tower icon, then click on an empty space in the map. Click on the new tower. Note that the prices below the "Sell" and "Upgrade" icons are offset.

Someone else who ran into this issue.
https://groups.google.com/forum/?fromgroups=#!topic/melonjs/ueUtLkh79Vc

[greghouston]

Note that the text in each instance is positioned using "this.pos" where "this" is the object.

[parasyte]

This one is by design. Position vectors are in world coordinates. You want to draw things in screen coordinates.

• To convert from world coordinates to screen coordinates, subtract the viewport position from the object position.
• To convert from screen coordinates to world coordinates, add the viewport position to the screen position.

[greghouston]

This is another one that seems really unintuitive, sort of IE6 style. What would be a use case where you want something drawn on an object to exist in relation to the viewport rather than the object? It seems like you would want to use an entity like the HUD that remains stationary in the viewport.

Along those line, is there some sort of toggle to make a SpriteObject or AnimationSheet stay stationary in relation to the viewport? I have some items that don't make sense as HUD items since they don't have values to update, but do make sense as sprites since they have a graphic, e.g., start, pause, heart, fast-forward, tower menu, etc.

[parasyte]

Drawing is always done in screen coordinates by default because you pretty much have direct access to the canvas.

There's a boolean property called floating which forces mouse event handlers to use screen coordinates, instead of world coordinates. I don't know of an easy way to reuse the property for drawing at screen coordinates. Maybe the draw manager can change the context.translate state when the object has the floating property set... But it could be very confusing to have two objects using the same position vector, but drawing at different locations on screen.

[parasyte]

I should point out that some of your HUD objects are not being drawn as part of the HUD_Item they belong to. Your HeartEntity extends ObjectEntity, which always draws itself in world coordinates. (But if you override the draw method to draw additional things, you will still have to convert the position vector to screen coordinates!) You should be drawing the heart in your LivesObject HUD_Item, instead.

[greghouston]

Just thinking out loud, but if I drew something in an object's draw function with no reference to the object, then yeah, I would probably expect it to be drawn in relation to the viewport, though it wouldn't occur to me to do this. I would try to find a way to get the object to remain stationary in relation to the viewport. However, if you specifically reference the object's position with this.pos then the expected behavior is going to be that what is drawn is drawn in relation to the object. I wonder how much trouble it would be to add the offset into this.pos whenever it is called from in the draw function so that 1. its way more intuitive, and 2. you don't have to write long values like this:

context.strokeText( "$" + this.tower.upgradePrice, this.pos.x + this.spritewidth * .5 - me.game.viewport.pos.x, this.pos.y + this.spriteheight + 30 - me.game.viewport.pos.y);

Regarding the HUD items, yeah, I mentioned that above:

"Along those line, is there some sort of toggle to make a SpriteObject or AnimationSheet stay stationary in relation to the viewport? I have some items that don't make sense as HUD items since they don't have values to update, but do make sense as sprites since they have a graphic, e.g., start, pause, heart, fast-forward, tower menu, etc."

That heart actually has a value, unlike several other items, I just wish the HUD item had a built-in image like sprite does, and didn't require a value.

[parasyte]

It's really a matter of convenience that SpriteObject position vector even uses world coordinates in the draw method... It does this just by doing the conversion to screen coordinates hidden within that function: https://github.com/obiot/melonJS/blob/master/src/entity/sprite.js#L301

We don't want to actually change the position vector to handle edge cases like this. That will cause many nasty unforeseen problems. ;) Instead, the draw manager can translate the viewport context if it needs to before running the object draw method. But again, I don't think it will really be more intuitive at all. Right now melonJS only expects that you understand you are drawing directly to the canvas. If this change is made, then the expectation is that you understand you are drawing in screen coordinates iff this.floating == true, else you are drawing in world coordinates. To me, that sounds less intuitive than what we have now!

Next, you can (and probably should) use the vector math methods on the vector objects to make your code more readable:

var pos = this.pos.clone().sub(me.game.viewport.pos);
context.strokeText(
    "$" + this.tower.upgradePrice,
    pos.x + this.spritewidth * .5,
    pos.y + this.spriteheight + 30
);

The "toggle" you're asking about is the floating property, but it's not implemented for draw ... only for handling mouse events. What I'm suggesting with the draw manager modification will take care of this... if it's something we want to do.

Yes, your heart does have a value; in LivesObject; but it's the LivesObject that should draw the heart. You created an ObjectEntity for the heart (which uses world coordinates!) and did the same for your tower buttons. These GUI objects should not be extending ObjectEntity, anyway, because they don't use any of the physics features provided by ObjectEntity. They also should not extend AnimationSheet, because they don't animate. They could almost extend SpriteObject, except that it draws the position vector using world coordinates... But what you really want to do is make the HUD_Item that holds their values also do the drawing.

[parasyte]

This likely does manifest itself as an actual bug in GUI_Objects. I haven't tried it yet, but it appears that GUI objects will be drawn in world coordinates (scrolled with the viewport) while their event rectangles remain in screen coordinates. If this is the case, the draw manager patch should be considered.

[greghouston]

I'm off to bed, but when I replace:

context.strokeText( "$" + this.tower.upgradePrice, this.pos.x + this.spritewidth * .5 - me.game.viewport.pos.x, this.pos.y + this.spriteheight + 30 - me.game.viewport.pos.y);

with:

```javascript
var pos = this.pos.clone().sub(me.game.viewport.pos);
context.strokeText(
    "$" + this.tower.upgradePrice,
    pos.x + this.spritewidth * .5,
    pos.y + this.spriteheight + 30
);

I get the following error:

TypeError: pos is undefined
[Break On This Error]   

pos.x + this.spritewidth * .5, 

[greghouston]

Oops. Hit the submit button instead of the Markdown link.

[parasyte]

I got a little carried away with the chaining operations. Looks like some of the math methods don't allow chaining at all. :(

var pos = this.pos.clone();
pos.sub(me.game.viewport.pos);
context.strokeText(
    "$" + this.tower.upgradePrice,
    pos.x + this.spritewidth * .5,
    pos.y + this.spriteheight + 30
);

[greghouston]

Quote:
// You should be drawing the heart in your LivesObject HUD_Item, instead.

Jason, are you sure I can use a HUD_Item since my heart animates?

This is my attempt, but am not really sure what I should be doing. When the animation is triggered, which is the same time the lives value should decrease the value turns to NAN and there is no animation. Also I once again moved the lifeLost and lifeTween to the jsApp namespace since I didn't know how to assign the instance of this HUD_Item to jsApp.LivesObject.

var LivesObject = me.HUD_Item.extend({
  init : function(x, y) {
    // call the parent constructor
    this.parent(x, y);
    this.scale = {mult: 1};

    // a tween to animate the heart   
    jsApp.lives.lifeTweenBack = new me.Tween(this.scale).to({mult:1}, 50).onComplete(function(){
      jsApp.lives.lifeLost = false;
    });
    jsApp.lives.lifeTween = new me.Tween(this.scale).to({mult:1.2}, 200).onComplete(function(){
      jsApp.lives.lifeTweenBack.start();
    });
    jsApp.lives.lifeTween.easing(me.Tween.Easing.Bounce.EaseOut);
    jsApp.lives.lifeLost = true;
    jsApp.lives.lifeTween.start();    
  },

  update : function() {
    if (jsApp.lives.lifeLost) {
      this.parent();
      return true;
    }
    else {
      return false;  
    }  
  },

  draw : function(context, x, y) {          
    //this.parent(x, y);
    context.font = '36px GoodDogRegular';
    context.textAlign = 'right';    
    context.lineWidth = 4;
    context.strokeStyle = '#000';
    context.strokeText( this.value, this.pos.x, this.pos.y );              
    context.fillStyle = '#fff';
    context.fillText( this.value, this.pos.x, this.pos.y );

    var heart = me.loader.getImage('lives');
    context.drawImage(heart, this.pos.x, this.pos.y), this.scale * 100, this.scale * 100;    
  }

});

This is the trigger:

jsApp.lives.lifeLost = true;
jsApp.lives.lifeTween.start();

[parasyte]

It most certainly is possible. I had to override the HUD_Object.update method to call a 'request_update' method added to each HUD_Item. 'request_update' has the same purpose as the 'update' method on typical game objects; it returns true when the HUD_Item wants to be redrawn. The HUD_Object will then invalidate itself so that it redraws.

It allows the coin in my HUD to animate. As well as other things.

That's how it works. Here's the code: https://bitbucket.org/parasyte/neverwell-moor/src/c6f5b8715267/js/objects/gui.js

[greghouston]

Cool, I'll check it out. Thanks.

Along these same lines, several of my HUD Items are just buttons, e.g., pause, mute, menu fastforward, and next wave.

When I try to add a onmousedown event to them I get this error:

TypeError: rect is undefined
[Break On This Error]

var _float = rect.floating===true?true:false;

Example:

var PauseObject = me.HUD_Item.extend({
  init : function(x, y) {

    // call the parent constructor
    this.parent(x, y);

    // register on mouse event
    me.input.registerMouseEvent('mousedown', this.collisionBox, this.onMouseDown.bind(this));

  }, 

  draw: function(context, x, y) {
    context.font = '36px GoodDogRegular';
    context.textAlign = 'left';
    context.lineWidth = 4;
    context.strokeStyle = '#000';
    context.strokeText( "pause", this.pos.x, this.pos.y );              
    context.fillStyle = '#fff';    
    context.fillText( "pause", this.pos.x, this.pos.y );      
  },

    // callback for mouse click
  onMouseDown: function() {

    if (gamePaused) {
      me.state.resume();
      gamePaused = false;
    }
    else {
      me.state.pause();
      gamePaused = true;  
    } 

    //stop propagating the event
    return false;
  },

});

[parasyte]

HUD_Item does not have a collisionBox. Use 'this' to refer to the rectangle (especially in the 'registerMouseEvent' call) I think that will do the trick? Else you can create a new me.Rect with ye same dimensions and pass that.

[greghouston]

Yeah, using "this" didn't work. I'll try the rectangle, but it should be noted that getting HUDs to function for buttons and with animated graphics is more of a work around than using SpriteObjects and AnimationSheets for them. Any non-trivial games, not just simple demos, are going to require these sort of HUD Items. Maybe there should be a Sprite HUD, an Animation Sheet HUD, and a button HUD. A button HUD should probably be a variation of AnimationSheets so it can use a different sprite when pushed.

[parasyte]

They are both workaround for the real problem; HUD_Items don't support animation or mouse events.

[greghouston]

Yeah, that's what I'm saying. Since these are not edge cases, games have buttons, and gui elements are very often animated in some way, there should be object types for them that don't require workarounds. The workaround code for the animated HUD looks particularly complex. I don't even know what any of it does.

[parasyte]

You can "cheat" a great deal by making HUD_Object.update always return true. That will remove some of the perceived complexity, by unconditionally redrawing every HUD_Item every frame, even unnecessarily.

The "complexity" comes from trying to be intelligent about when the update method returns true, mainly when one of the child HUD_Items wants to be redrawn. The game manager does this for "normal" game objects, but HUD_Object is a special case because it has its own children objects that need similar treatment.

Anyway, there's also the GUI_Object, which extends SpriteObject, and is meant more for interactive GUI elements like buttons. But as I mentioned yesterday, I'm fairly confident they also draw using world coordinates, unlike HUD_Object.

[parasyte]

One more thing: You should not override HUD_Item.update like that. HUD_Item.update is for updating the value. It's very different from the update method on other classes. I think it's actually a problematic inconsistency (but that's unrelated to this ticket).

[greghouston]

Ah, I missed the GUI_Object entirely. It even has an example button, but I
would definitely think it should act like a HUD item and stay in the
viewport by default. A bonus event for this object would be an active state
that occurs while pressed so you could change the current sprite position.
Hmm, maybe you can only change sprite position on AnimationSheets. It's too
bad it's not easier to mix and match the features of these different
Objects. I've obviously had a lot of difficulty with them.

Again just thinking out loud, but a super flexible object might just have
toggles for all these different features. I don't know if it could
intelligently pull in the necessary functionality based on the settings so
that each object didn't have to have all the functionality in memory doing
nothing unless that functionality was set to true.

clickable: true,
itemValue: true,
coordinateSystem: screen,
sprite: true,
animationSheet: true,
...

It seems like every object should be able to have a sprite, have an
animation sheet, be clickable. Even my ObjectEntity's have a value,
"health", but only HUD items have built-in updatable value. It seems like
pretty much every Object I've created needs a feature a different type of
object has.

On Tue, Sep 25, 2012 at 2:38 AM, Jason Oster notifications@github.comwrote:

You can "cheat" a great deal by making HUD_Object.update always return
true. That will remove some of the perceived complexity, by unconditionally
redrawing every HUD_Item every frame, even unnecessarily.

The "complexity" comes from trying to be intelligent about when the update
method returns true, mainly when one of the child HUD_Items wants to be
redrawn. The game manager does this for "normal" game objects, but
HUD_Object is a special case because it has its own children objects that
need similar treatment.

Anyway, there's also the GUI_Objecthttp://www.melonjs.org/docs/symbols/me.GUI_Object.html,
which extends SpriteObject, and is meant more for interactive GUI elements
like buttons. But as I mentioned yesterday, I'm fairly confident they also
draw using world coordinates, unlike HUD_Object.

—
Reply to this email directly or view it on GitHubhttps://github.com/obiot/melonJS/issues/119#issuecomment-8845948.

[parasyte]

Mix-and-match object features sounds a lot like Ticket #15.

I think we should leave this ticket for its specific purpose, though. And I think fixing it requires using screen coordinates for floating objects, and all other objects use world coordinates. I'll do a quick patch, and seek some feedback. I still don't know if it's something we will end up doing. But at least there will be a patch for it.

As for the other stuff, there are some objects that do not draw themselves. The ScreenObject is an example; by default the draw method is empty. Plugin objects are another example (Ticket #85).

[greghouston]

Just to make sure I'm following. Would you set this.floating = true on a
SpriteObject, AnimationSheet, or GUI_Object to make it use the screen
coordinates or am I misunderstanding?

[parasyte]

GUI_Object already sets this.floating = true: https://github.com/obiot/melonJS/blob/master/src/GUI/GUI.js#L71 And you can set it on any "game manager object" you like. But it doesn't make sense to use on all objects. ScreenObject wouldn't benefit from it, for example.

[greghouston]

Yeah, when I'm talking about objects I'm just refering to those that are visible in the screen. My "flexible" object obviously would not include stuff like the ScreenObject or Plugin objects. Those are entirely different beasts that don't need to borrow features from sprites, animation sheets, object entities or gui objects.

[greghouston]

So I just tried out a GUI_Object with an offset viewport and the click event is offset from the GUI_Object's graphic. Maybe the proposed patch will fix that as well.

[parasyte]

Thank you for the confirmation.

[parasyte]

Here is a demo of this bug, using GUI_Object: http://jsfiddle.net/E6vAs/

[parasyte]

Updated demo, added a "non-floating" SpriteObject at world coordinates 100,0. The GUI_Object is at screen coordinates 100,100, and the viewport is at position 100,0. http://jsfiddle.net/E6vAs/1/

The expected output should place the SpriteObject in the corner at 0,0, and the GUI_Object at 100,100.

Here is the same demo with an updated melonJS using a potential patch, which I will be committing to a new branch shortly: http://jsfiddle.net/E6vAs/2/

[parasyte]

TODO: me.ImageLayer, me.TMXLayer

[parasyte]

Important caveat: This patch will break all code that currently expects that it should be drawing everything in screen coordinates. All tutorials, all demos, all existing games have a potential to be broken by this patch.

[melonjs]

yep, just noticed it :)

I guess it's a bad thing for a good thing ?

[parasyte]

This could wait for version 1.0 when we decide to break other things too. :)

[melonjs]

Well to be honest I'm perfectly fine with breaking this now :)

I’m for sure always trying to maintain backward compatibility with previous versions (to ease people life when upgrading), but sometimes we need to break something to make a step forward. The other point is that if we wait for version 1.0, considering that right now the development cycle is around 3 months per version, this won’t happen before next year ! and with the right communication (through changelog, forum, and release announcement) any major troubles can be avoided/managed.

Additionally, I’m hoping for Tiled 0.9 to be available any time soon now, and once the case, I will only allow the new parallax code based on the Image Layer, which will break anyway compatibility with any games released so far 

I was thinking, what about actually adding a section in the wiki, that would list the major API changes per version (having in mind code upgrade from a game point of view) ?

[melonjs]

else additional TODO(s) :

• entity (entityObject, InvisibleEntity) debug code (renderHitBox)
• me.Rect draw function (debug purpose as well)
• HUD_Object draw function

[parasyte]

Yeah, this will really need a more thorough audit to ensure everything is correct. If you're ok with the breaking change, I'm ok with it too!

Wiki could be quite helpful for tracking Roadmap goals and noting backward compatibility issues that arise.

[melonjs]

@parasyte I just made a bunch of commits for this ticket (i forgot to reference the ticket), finished testing what I have right now, and I'm not seeing anything wrong (once the floating property is added with the required value).

On your side, could you give a round of testing too ?

Do we finally commit this on the 0.9.5 master branch ? for me it's ok, assuming we carefully document it :)

[melonjs]

@parasyte I actually don't know how I did that, but I accidently merge the ticket-119 branch into the master one :)
I guess that means that the changes are now official... I tried to do some rollback, but he does not want to...

[parasyte]

oh, hehe. I can revert it, if you like. The only thing "bad" about reverts is it makes the commit history messy. :)

I want to revisit this later, after ticket #59. I was never happy with adding the save/translate/restore around every object.draw(). That makes me sad. With #59, I think we can create a "GroupObject" class which stores child objects to delegate the update and draw methods to its children. So all "world coordinate objects" can be placed into a GroupObject by default, and the GroupObject.draw() can do the save/translate + iterate all children.draw() + restore. That will be much faster, especially with a lot of "non-floating" objects (using world coordinates).

What do you think?

[melonjs]

so can this one be closed as well now ?

[parasyte]

Yep, let's close this with #135 out of the way. :)

[melonjs]

note: I'm "killing" the #119 branch on github as well :)