New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An update for TerrainUtils #67

Merged
merged 25 commits into from Jun 19, 2018

Conversation

Projects
None yet
3 participants
@Trokkin
Copy link
Contributor

Trokkin commented Apr 28, 2018

No description provided.

@Cokemonkey11

This comment has been minimized.

Copy link
Contributor

Cokemonkey11 commented Apr 29, 2018

Looks pretty good. Maybe squash?

@@ -28,6 +28,21 @@ public function hypot(real dx, real dy) returns real
x = y
return x * SquareRoot(1 + t * t)

/** Returns value of 'a' rounded up or down to 'scale' */

This comment has been minimized.

@Frotty

Frotty May 1, 2018

Member

use toInt() and toReal()


public function isTerrainWalkable(real x, real y) returns boolean
//Hide any items in the area to avoid conflicts with our item
MoveRectTo(find, x, y)
EnumItemsInRect(find ,null, function hideItem)
EnumItemsInRect(find, null) ->
if IsItemVisible(GetEnumItem())

This comment has been minimized.

@Frotty

Frotty May 1, 2018

Member

use .isVisible() and other item extension funcs

EnumItemsInRect(find, null) ->
if IsItemVisible(GetEnumItem())
hid[hidMax] = GetEnumItem()
SetItemVisible(hid[hidMax], false)

This comment has been minimized.

@Frotty

Frotty May 1, 2018

Member

create item ext func if it doesn't exist

SetItemVisible(hid[hidMax], false)
hidMax++
public function vec2.isTerrainPlatform() returns boolean
return isTerrainPlatform(this)

public function isTerrainWalkable(real x, real y) returns boolean

This comment has been minimized.

@Frotty

Frotty May 1, 2018

Member

iirc there was a new method on hive that doesn't use an item. Could perhaps be added in this package.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 4, 2018

Contributor

Source?


/** represents a single terrain tile */
public tuple tile(int id)
let tilesX = R2I(boundMax.x - boundMin.x) div 128 + 1

This comment has been minimized.

@Frotty

Frotty May 8, 2018

Member

extension funcs? UPPERCASE since constant @Trokkin will u update this soonish?

Trokkin added some commits May 9, 2018

@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented May 9, 2018

So, @Frotty, it is updated though not tested because I have no possibility at the moment.
Though as always I do say that it should work.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented May 9, 2018

Build fails, so I doubt that :)
Thanks for update though

@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented May 9, 2018

Err, idk why my ide didn't show me that errors
Hey, I don't like that I can't add pretty much any package due to cyclic dependencies. I assume that getterrainZ should be moved into Vector package.

@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented May 11, 2018

Moved .getTerrainZ() into Vectors package just to be sure that cycles won't happen.
I resolved all the errors, optimized the code not to have repeating functions and corrected usage in different packages.

@Frotty
Copy link
Member

Frotty left a comment

Again, a very big PR Trokkin 🥇
This really slows down the review process.
I think system functionality, actual usage, and fixes like moving getTerrainZ() into vector should all be their own PR.
Other than that, seems like a nice addition, but the tile tuple framework should be unit tested.

@@ -28,6 +28,21 @@ public function hypot(real dx, real dy) returns real
x = y
return x * SquareRoot(1 + t * t)

/** Returns value of 'a' rounded up or down to 'scale' */
public function round(real a, real scale) returns real

This comment has been minimized.

@Frotty

Frotty May 12, 2018

Member

round already exists as extension funcs - what these should be as well.

MoveLocation(tempLoc, this.x, this.y)
return GetLocationZ(tempLoc)
/** Fills 32x32 rect around vec2 with given pathing type
min = vec2(roundDown(x,32), roundDown(x,32))

This comment has been minimized.

@Frotty

Frotty May 12, 2018

Member

Why is this in a comment

public function tile.getY() returns real
return (this.id div TILES_X) * 128. + boundMin.y

// vec2 convertion

This comment has been minimized.

@Frotty
@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented May 13, 2018

Tile framework completely depends on MapBounds package, since it requires minimum and maximum of the map in order to index all tiles in the map, so, while MapBounds cannot be somehow tested from compiletime, neither can Tile.
Yeah, it's a big PR, but it's all about one package, and I don't agree that it should be divided. What you thought is like creating a PR for each commit to work-in-progress feature. And if every next PR will depend on previous, this will slow down development speed much more.
Well, getTerrainZ() move could be in other PR, if only it wouldn't be caused by a cycle appeared when I pulled updates from master.

Trokkin added some commits May 13, 2018

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented May 14, 2018

Tile framework completely depends on MapBounds package, since it requires minimum and maximum of the map in order to index all tiles in the map, so, while MapBounds cannot be somehow tested from compiletime, neither can Tile.

This Test now runs successfully 80110a3#diff-9215e1d6aaac66c35bfa08182de283b1R63
If you need anything else let me know

What you thought is like creating a PR for each commit to work-in-progress feature

No, not work in progress, but system implementation can be decoupled from actual usage, as well as unrelated extension functions and asset paths. The assets and extension functions in this PR I would insta merge. But since all this other stuff is also here, I can't.

@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented May 16, 2018

Er, that update to mapbounds seems like a workaround to me. Well, nevermind.

Ok, I got your idea.
Anyway, is there anything else left to do here aside from unit tests?


constant MAX_RANGE_SQ = 10.*10.
constant DUMMY_ITEM_ID = 'wolg'
let dItem = CreateItem(DUMMY_ITEM_ID, 0, 0)..setVisible(false)

This comment has been minimized.

@Frotty

Frotty May 17, 2018

Member

createItem(DUMMY_ITEM_ID, ZERO2)

constant MAX_RANGE_SQ = 10.*10.
constant DUMMY_ITEM_ID = 'wolg'
let dItem = CreateItem(DUMMY_ITEM_ID, 0, 0)..setVisible(false)
let find = Rect(0., 0., 128., 128.)

This comment has been minimized.

@Frotty

Frotty May 17, 2018

Member

these constants should adhere to naming convention. Change should be fine since they are not public.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented May 17, 2018

Er, that update to mapbounds seems like a workaround to me. Well, nevermind.

Supplying mock values at runtime is somewhat a workaround, yes. At some point we might be able to read in those values from wurst.build, but for now this shall suffice.
As long as the tests verify everything works as you expect it to on that mock data, it's already a big win.

Other than that and a couple comments I think this is ready.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented May 27, 2018

so it's done?

@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented May 29, 2018

Yes, it is,...
...aside from extraneous tests for wc3's specific terrain functions that do not exist in wurst compiletime equivalent, which we discussed back then. If you did not add support for that terrain functions, then I can't test nothing more.

And, by the way, there's no unittesting for vectors or results of returns nothing functions. That stops me from testing tile.toVec2().

@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented Jun 3, 2018

So, @Frotty, will you merge?

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jun 3, 2018

I was away this week. Will take a look in the next few days.

boundMin.getTile().id.assertEquals(0)
boundMax.getTile().id.assertEquals(TILES_X * TILES_Y - 1)
tile(boundMax.x, boundMin.y).id.assertEquals(TILES_X - 1)
tile(boundMin.x, boundMax.y).id.assertEquals(TILES_X * (TILES_Y - 1))

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 4, 2018

Contributor

Missing EoF LF

@Cokemonkey11

This comment has been minimized.

Copy link
Contributor

Cokemonkey11 commented Jun 4, 2018

LGTM

@@ -157,4 +157,3 @@ public function min(real a, real b, real c, real d, real e, real f) returns real

function test_hypot_help(real x, real y)
hypot(x,y).assertEquals(SquareRoot(x*x + y*y), 0.0001)

This comment has been minimized.

@Frotty

Frotty Jun 10, 2018

Member

? @Cokemonkey11 will kill you for removing eof newline

This comment has been minimized.

@Trokkin

Trokkin Jun 10, 2018

Author Contributor

wtf, there's a newline if I edit this both online and on my PC

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member

then reset the Maths.wurst file.

@@ -81,6 +81,21 @@ public function vec2.rotate(angle angl) returns vec2
public function vec2.toVec3() returns vec3
return vec3(this.x, this.y, 0.)

let tempLoc = Location(0.,0.)
public function vec2.getTerrainZ() returns real
MoveLocation(tempLoc, this.x, this.y)

This comment has been minimized.

@Frotty

Frotty Jun 10, 2018

Member

MoveLocation extension func?

This comment has been minimized.

@Trokkin

Trokkin Jun 10, 2018

Author Contributor

eh? there are no extension funcs for locations in the entire stdlib, and I won't add anything else to this already huge PR

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member
  • next PR
@@ -21,6 +21,21 @@ public function real.sign() returns int
public function real.round() returns int
return this > 0 ? (this+.5).toInt() : (this-.5).toInt()

/** Rounds the input real to the nearest 'scale' divisible */

This comment has been minimized.

@Frotty

Frotty Jun 10, 2018

Member

These hot docs are kinda cryptic. Either use actual mathematical vocabulary or explain what it does in natural text.
Also, why are the round functions not returning int as one would expect?

This comment has been minimized.

@Trokkin

Trokkin Jun 10, 2018

Author Contributor

Because it properly divides by any real. E.g. (3,9).round(1,5) == (4,5)

Huh, I thought divisibility is a term from math, at least it was in my russian math course.
How about Returns the nearest real divisible by 'scale'?

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member

Yeah, one usually expects round/floor functions to return ints, not reals.
In any case, I'm not sure if the functions in general are a good fit. If I were to search for such functionality, I wouldn't search for "round". When I see .round(412) I'm not immediately understanding what exactly the parameter means. And since it's a simple one-liner it's usually implemented in-place anyway.
Do you even make use of roundUp/Down with scale yourself? Do you have any examples of this in other libraries?

Also, divisible might be a term, but I haven't seen it used in this context. Something like "Returns the nearest multiple of 'scale' from the input number" strikes me with much more understanding.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jun 10, 2018

sry forgot to submit a pending review from 3-4 days ago.
Otherwise looks good now. Perhaps there can be some more integration of this in other packages with another PR.

@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented Jun 11, 2018

With another PR, that's it.
I definitely need to learn how to make PRs with lifetime less than a month.
@Frotty, you can merge this.

tile(boundMax.x, boundMin.y).id.assertEquals(TILES_X - 1)
tile(boundMin.x, boundMax.y).id.assertEquals(TILES_X * (TILES_Y - 1))

@test public function testUpperBound()

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member

don't make test functions public, otherwise they will show up in autocomplete

Trokkin added some commits Jun 16, 2018

Made test functions private
(online update, not tested)
@Trokkin

This comment has been minimized.

Copy link
Contributor Author

Trokkin commented Jun 18, 2018

It's ok to merge now.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jun 18, 2018

you didn't even reply to comment

@Frotty

Frotty approved these changes Jun 19, 2018

@Frotty Frotty merged commit cd05b0a into wurstscript:master Jun 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jun 19, 2018

🍬 finally

@Trokkin Trokkin deleted the Trokkin:terrainUtils branch Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment