Pryaxis / TShock

☕️⚡️TShock provides Terraria servers with server-side characters, anti-cheat, and community management tools.
GNU General Public License v3.0
2.41k stars 377 forks source link

[#590] Regions are missing bottom and right within Contain #1161

Closed Marcus101RR closed 8 years ago

Marcus101RR commented 8 years ago

The Commit: https://github.com/NyxStudios/TShock/commit/f0bf0ad82098e2cbdd818f4391368544e6f2a102 TShock Unstable #590

Breaks region, since you the new method Contain is used it leaves out the last bottom and last right side of the region, so you must calculated -1 on both ends. You can test this by created a region that is exactly one point, and then simply created a new command that calls the Area.Contains(x, y) method directly for that region. It should not appear. A result: image

The wooden 2x4 block placed here on the Left (#585) remains intact, while the one on the Right (#590) is taken away. To further explain, the wooden block is located in a region "Housing" that is buildable, and as such will not be overwritten by any pasting, or regeneration.

I didn't come to this until someone reported that my plugin World Regeneration was destroying their housing area, after which I added it to mine and it did the same thing. Seems Contains doesn't include the bottom and right most area of the rectangle...

EDIT: The fix to this problem is adding +1 to both the width/height of each region in your database, may be extremely tedious for those who have a lot.

NOTE: The /region set command doesn't account for this error margin. Advise to either make the command +1 on width/height, or add +1 to contain method.

QuiCM commented 8 years ago

The listed commit changes regions from using our own math calculation (point.X <= region.Area.Left ...) to using the XNA Rectangle struct's Contains method. The issue lies in the following:

struct Rectangle {
    bool Contains(int x, int y)
    {
        return this.X <= x &&         //this is fine
        x < this.X + this.Width &&    //this cuts off (old method used equivalent of 'x <= this.X + this.Width')
        this.Y <= y &&                //this is fine
        y < this.Y + this.Height;     //this cuts off (old method used equivalent of 'y <= this.Y + this.Height')
    }
}

@nicatronTg @Ijwu @tylerjwatson should this issue be resolved by directly modifying the XNA Rectangle struct we have in TerrariaServer, or should we put a work-around into the Region class here to make regions function as they used to

Marcus101RR commented 8 years ago

If you like contain, why not just edit the system. So that Region /region set/define adds +1 to the width/height. Sadly, still requires all regions to be +1 in the database.

QuiCM commented 8 years ago

Sadly, still requires all regions to be +1 in the database.

The intention was never to break existing regions, so this isn't really an option. Changing the contains method here would be easiest, but would also require a change to Contains(Point) for consistency. However, Contains(Point) is used by worldgen so changing it could cause unexpected results.

Marcus101RR commented 8 years ago

This truly is a question on how to resolve this without breaking the existing regions, and using a more cleaner/simpler form of checking inarea.

QuiCM commented 8 years ago

Well, double committed by accident, but it should be fixed