ID:114439
 
Resolved
Client-side icon operations using DrawBox() or SwapColor() followed up by something that could change the icon's size could confuse Width() and Height() into returning incorrect values.
BYOND Version:483, 484, 485
Operating System:Windows XP Pro
Web Browser:Opera 11.10
Applies to:Dream Seeker
Status: Resolved (486)

This issue has been resolved.
This may be or may be related to Zaltron's Issue: 3121.


Problem Description:
In The Saloon we were discussing a few interface design points and upon showing off my ideas in Ultimate Jigsaw I came to find that the game was no longer playable due to issues with icon operations. Upon investigating which version it became an issue with, I found something even more upsetting.

Unfortunately, the icon operations in Ultimate Jigsaw are numerous and heavily condensed, I don't quite have the full source at my disposal, and I don't have the time to set up tests, so I can't give too much information through code examples. I can explain the way that connecting pieces works, if that helps.

When a piece connects to another piece, it will determine whether or not the old piece(s) plus the new piece(s) will be larger vertically or horizontally. If not, it will simply Blend() the new piece(s) into the correct position on the old piece(s) icon. If so, will create a new blank icon to correctly accommodate both and then blend both the old and new piece(s) into the correct positions on the new icon, discarding the leftovers.

Version Testing:
482: Game plays fine, runs fine, all is well.

483: Game plays fine but as the size of the icons grow, i.e., the more pieces you connect together vertically and/or horizontally, the slower and slower the operations work. It's quite noticeable and compounds quickly.

484 & 485: Game doesn't play at all. If you connect pieces together, they don't seem to Scale() properly and end up cutting off the ends of pieces.

EDIT: I dug out my laptop and a replacement monitor and set it up on the washer just for you. Here's the portion of the source that pertains to this issue.
Connect(client/C)
for(var/Piece/P in C.screen)
if(P==src || !length(P.connected&connectable) || rotation!=P.rotation || map_loc != P.map_loc) continue
var{newx;newy;ix;iy;sx;sy;nx;ny;ps=C.piece_size+C.piece_split;colnum=(col-P.col)*ps;rownum=(row-P.row)*ps;icon/i=new(P.icon)}
switch(rotation)
if(0) {newx=P.pixel_locx+colnum;newy=P.pixel_locy+rownum;ix=colnum;iy=rownum}
if(1) {newx=P.pixel_locx+rownum;newy=((P.pixel_locy+P.sizey)-colnum)-sizey;ix=rownum;iy=(P.sizey-colnum)-sizey}
if(2) {newx=((P.pixel_locx+P.sizex)-colnum)-sizex;newy=((P.pixel_locy+P.sizey)-rownum)-sizey;ix=(P.sizex-colnum)-sizex;iy=(P.sizey-rownum)-sizey}
if(3) {newx=((P.pixel_locx+P.sizex)-rownum)-sizex;newy=P.pixel_locy+colnum;ix=(P.sizex-rownum)-sizex;iy=colnum}
if(!(pixel_locx<newx+C.piece_split&&pixel_locx>newx-C.piece_split)||!(pixel_locy<newy+C.piece_split&&pixel_locy>newy-C.piece_split)) continue
i.DrawBox(null,1,1,i.Width(),i.Height())
if(ix<0) sx=abs(ix);if(iy<0) sy=abs(iy)
if(sx||sy) {i.Scale(i.Width()+sx,i.Height()+sy);P.PixelMove(C,sx ? ix : 0, sy ? iy : 0,TRUE)}
nx=max(ix+sizex,sizex,i.Width());ny=max(iy+sizey,sizey,i.Height());i.Scale(nx,ny)
i.Blend(new/icon(P.icon),ICON_OVERLAY)
if(sx)i.Shift(EAST,sx);if(sy)i.Shift(NORTH,sy)
i.Blend(new/icon(icon),ICON_OVERLAY,sx?1:ix+1,sy?1:iy+1);P.Icon(C,null,i)
world << sound('click.ogg')
var/list/Pconnectable = P.connectable&connectable+P.connectable|connectable
var/list/PConnected = P.connected&connected+P.connected|connected
Pconnectable -= Pconnectable&PConnected;P.connectable=Pconnectable;P.connected=PConnected
P.col=min(col,P.col);P.row=min(row,P.row)

"The problem is somewhere in there."
I'll need something to reproduce. Without that this bug can't be investigated.
I've had this issue as well with poor performance relating to the Blend proc.

I'm using DMIFONTSPLUS to draw text using a colour and black outline.
In 482.1091 the Blend proc would use about 0.001% self CPU.

In 485 the self CPU would be very near 0.90%.
While processing this operation the server would freeze until it can complete.

Behaviour observed while in a local dream seeker hosted environment with no outside connections, just the host.

If you need more information please let me know, hopefully I can provide more.
The code snippet is potentially helpful, but you're calling other routines in there whose purpose I don't know (like PixelMove()). As long as I can strip out everything not related to icons I can test with it, but it's important that I know what the expected vs. actual results are.
Well, did you actually play the game through the three versions of BYOND? It's kind of a visual effect.

482 = Expected Results
483 = Gradual Slowdown
484,485 = Broken Icon Operations

PixelMove() does nothing but change the screen_loc of objects and the rest of it should just be math. Everything that happens when the issues occur happens right there.
Alright, I had a bit today and I have pulled out all the lines that contain icon related operations. Without really knowing what it all does, the math shouldn't mean anything anyways, so I've tried to pull that out as well. What's left are a few variables that can range from 1~500.

var/icon/i=new(P.icon)
i.DrawBox(null,1,1,i.Width(),i.Height())
if(sx||sy) {i.Scale(i.Width()+sx,i.Height()+sy)
nx=max(ix+sizex,sizex,i.Width());ny=max(iy+sizey,sizey,i.Height());i.Scale(nx,ny)
i.Blend(new/icon(P.icon),ICON_OVERLAY)
if(sx)i.Shift(EAST,sx);if(sy)i.Shift(NORTH,sy)
i.Blend(new/icon(icon),ICON_OVERLAY,sx?1:ix+1,sy?1:iy+1)
//From P.Icon(C,null,i)
P.icon = i
P.sizex = i.Width()
P.sizey = i.Height()
That condensed snippet was enough for me to verify there was an issue and solve it. Apparently, DrawBox() and SwapColor() were throwing off calculations of Width() and Height().

I should mention for efficiency's sake that using Scale() on a blank icon is non-ideal, when you could use Crop() instead. (Call Crop(1,1,w,h) instead of Scale(w,h).) The double call is also unnecessary, since you can simply lose the first Scale() and calculate the size you want as sx+i.Width() and sy+i.Height() in place of i.Width() and i.Height() in the nx and ny calculations. Those changes may have an impact on speed as well.
Hm, I guess I never considered that Crop() could increase the size of the icon. Thanks for the tip. Also, holy shit, I didn't even realize that I had done that with Scale(). I can't even imagine why... When I finally update it, I'll be sure to take care of that, along with quite a few other issues.

On the other side of the bug report, did you figure out what was causing the speed issues? Before the problems with DrawBox() and SwapColor() in 484, there are major speed issues in 483.
Honestly I haven't noticed any speed issues with DmiFontsPlus, because it pretty much always hits the opstack limit very early on. Because creating the opstack itself is insignificant, and evaluating it to convert to a regular server-side icon takes no more time than doing those operations would have prior to 482, there shouldn't be any hit with those.

However, I do think it's conceivable that the errors in question here were confusing the logic in particular code in such a way as to cause problems.
But the speed issues occurred in a version prior to the version with the corrected errors. The speed issues occurred in 483 and the DrawBox() and SwapColor() issues started in 484.

Seriously, complete an easy puzzle in Ultimate Jigsaw in version 482. Then try the same thing in 483. The issue is very noticeable.
Width() and Height() took much longer to complete in 483 and 484 because of the icon changes. The speed issue caused by those was remedied in 485. You're using both functions to an excessive degree, which I think is responsible for the slowdown being so exaggerated.
Okay. I can accept that.