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

Create Camera.wurst #80

Merged
merged 3 commits into from Jun 10, 2018

Conversation

Projects
None yet
4 participants
@Ouguiya
Copy link
Contributor

Ouguiya commented May 28, 2018

Initial proposal - Includes wrappers for almost all camera functions from common.j and blizzard.j. Functions with (real x, real y) and functions with (location loc) have been replaced by vec2. One function (smartCameraPan) was reimplemented from the blizzard.j function.

Create Camera.wurst
Initial proposal - Includes wrappers for almost all camera functions from common.j and blizzard.j. Functions with (real x, real y) and functions with (location loc) have been replaced by vec2. One function (smartCameraPan) was reimplemented from the blizzard.j function.
return vec2(GetCameraTargetPositionX(), GetCameraTargetPositionY())

public function getCameraTargetPositionVec3() returns vec3
let targetPosition = getCameraTargetPositionVec2()

This comment has been minimized.

@Frotty

Frotty May 28, 2018

Member

-> getCameraTargetPositionVec2().withZ(GetCameraTargetPositionZ())

This comment has been minimized.

@Ouguiya

Ouguiya May 28, 2018

Author Contributor

Great idea! Implemented.

public function getCameraBoundMax() returns vec2
return vec2(GetCameraBoundMaxX(), GetCameraBoundMaxY())

public function getCameraTargetPositionVec2() returns vec2

This comment has been minimized.

@Frotty

Frotty May 28, 2018

Member

Don't end function names with "vec", it's the default. Position is enough.

This comment has been minimized.

@Ouguiya

Ouguiya May 28, 2018

Author Contributor

Changed getCameraTargetPositionVec2 to getCameraTargetPosition and getCameraTargetPositionVec3 to getCameraTargetPosition3
Same with CameraEye

public function setCameraQuickPosition(vec2 pos)
SetCameraQuickPosition(pos.x, pos.y)

public function setCameraBounds(vec2 corner1, vec2 corner2, vec2 corner3, vec2 corner4)

This comment has been minimized.

@Frotty

Frotty May 28, 2018

Member

use proper names for the corner points, i.e bottomleft, topRight, etc.

public function panCameraToTimed(vec2 pos, real duration)
PanCameraToTimed(pos.x, pos.y, duration)

public function panCameraToWithZ(vec2 pos, real zOffsetDest)

This comment has been minimized.

@Frotty

Frotty May 28, 2018

Member

why not overload panCameraTo with vec3?

This comment has been minimized.

@Ouguiya

Ouguiya May 28, 2018

Author Contributor

I actually thought about it, but if I understand the function correctly, this does not pan to a specific point in 3D space. It pans to a 2D space and raises or lowers the camera by the amount or zooms the camera in or out (not sure which, but I assume this is why it's real zOffsetDest). The original function signature is: "native PanCameraToTimedWithZ takes real x, real y, real zOffsetDest, real duration returns nothing"

So here, I am worried that using vec3 might give the wrong impression.

This comment has been minimized.

@Trokkin

Trokkin May 29, 2018

Contributor

It does pan to Z, meaning if there would be duration, then camera movement will be smooth. Yes, Z offset will stay and yes, it is not absolute Z, because wc3's camera is always relative to terrain height. But it's still a certain position, which can be translated to absolute 3D coords by adding terrain height to Z.
But in wurst, vec3 is used in two ways: in absolute 3D space and relatively to terrain height. So the vec3 for your function will be the second type, and so there won't be any wrong impressions.

And even if I'm not right, you can forestall that impression by a hotdoc describing that feature.

This comment has been minimized.

@Ouguiya

Ouguiya May 29, 2018

Author Contributor

@Trokkin I'm a bit confused by your explanation here. The description for one of these types of functions in GUI reads: "Pan camera to (Location) with height x.xx above the terrain over y.yy seconds. The camera will not drop below the terrain height during its camera path."

What would I need to do to achieve the same thing in Camera.wurst?

i.e., if I wrote:

public function panCameraTo(vec3 pos)
    PanCameraToWithZ(pos.x, pos.y, pos.z)

I'm not sure this would produce the correct result. What would I need to do here?

This comment has been minimized.

@Trokkin

Trokkin May 31, 2018

Contributor

actually you could see in what JASS code that GUI functions are translated (they're actually BJ functions from Blizzard.j, btw) and look for anything suspicious.
As I know, the result would be correct, if you mean pos.z as the height above terrain, which is still a correct usage for vec3 as I explained.

This comment has been minimized.

@Ouguiya

Ouguiya May 31, 2018

Author Contributor

Unfortunately, that doesn't exactly help here. Here is the JASS code:

function PanCameraToTimedLocWithZForPlayer takes player whichPlayer, location loc, real zOffset, real duration returns nothing
    if (GetLocalPlayer() == whichPlayer) then
        // Use only local code (no net traffic) within this block to avoid desyncs.
        call PanCameraToTimedWithZ(GetLocationX(loc), GetLocationY(loc), zOffset, duration)
    endif
endfunction

And PanCameraToTimedWithZ is a native, so I can't see what it does.

Now if you tell me that vec3.z component automatically defaults to "height above terrain", then it should be fine. Otherwise, I'm not sure how to force vec3 to use terrain height instead of absolute.

This comment has been minimized.

@Trokkin

Trokkin Jun 3, 2018

Contributor

So, the description should apply 1:1 to that native, as it describes a simple wrapper without any hidden logic.

I told you, that's uncertain: vec3.z is used in both ways as relative and as absolute height, depending on function. See package Unit and how vec3 is used there, if you're curious or to find out how to convert back and forth absolute to relative heights. Exactly you want to see getTerrainZ() usage.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 4, 2018

Contributor

Can I ask you guys to make sure you document this in lep/jassdoc?

public function player.panCameraToTimed(vec2 pos, real duration)
PanCameraToTimedForPlayer(this, pos.x, pos.y, duration)

public function player.smartCameraPan(vec2 pos, real duration)

This comment has been minimized.

@Frotty

Frotty May 28, 2018

Member

Move single line comments into hotdoc and explain what the func does

This comment has been minimized.

@Ouguiya

Ouguiya May 28, 2018

Author Contributor

Done

Update Camera.wurst
- Changed name of functions ending in Vec2
- Used .withZ where appropriate
- Gave names to corner1, corner2, etc.
- Wrote hotdoc and explained smartCameraPan
@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jun 1, 2018

You should make a ticket for the overloading issue that stops the build from succeeding, as we discussed :)

public function camerasetup.getFieldSwap(camerafield whichField) returns real
return CameraSetupGetFieldSwap(whichField, this)

public function player.cameraSetupApply(boolean doPan, camerasetup whichSetup, real duration)

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 4, 2018

Contributor

I'm not really in love with some of these player extension-functions.

This comment has been minimized.

@Ouguiya

Ouguiya Jun 4, 2018

Author Contributor

If you want me to change anything, you will have to tell me what exactly bothers you about them.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 4, 2018

Contributor

I don't think I'm in a position to demand changes, but I wonder if someone else agrees that player extension-functions that are specific to camerasetup or otherwise camera handling might belong in a separate package that is imported explicitly.

This comment has been minimized.

@Frotty

Frotty Jun 4, 2018

Member

I think it's fine as long as this package is an auto import, which it should be.

This comment has been minimized.

@Trokkin

Trokkin Jun 4, 2018

Contributor

eh, I feel like it should be camerasetup's extension func instead of player's

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 5, 2018

Contributor

Frotty my point about using a separate package imported explicitly is the opposite of what you just said.

In particular, I think Player(3).panToTimed(vec2(0., 0.), 0.) is weird to have on auto-import.

This comment has been minimized.

@Ouguiya

Ouguiya Jun 5, 2018

Author Contributor

@Trokkin That already exists. I made both versions (for this function and a few others). The one you are looking for is camerasetup.apply, where you specify for which player to do this.

@Cokemonkey11 The function you just mentioned doesn't seem to exist? player.panToTimed isn't in the file. The function is called player.panCameraToTimed, which to me makes sense: Player(3).panCameraToTimed(pos, duration) = For Player 3, pan camera to pos timed over duration.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 5, 2018

Contributor

I think both are weird

This comment has been minimized.

@Frotty

Frotty Jun 5, 2018

Member

Just saying "it's weird" doesn't help, coke. I think it's totally fine, and I don't see what your problem is. When thinking naturally about this occuring in code, applying a camera position almost always happens together with other player related context. E.g. selecting a unit, adding some resources, etc. and in the same way I would expect to change the player's camera.
Camerasetup is seldom used, and calling an extension func on it wouldn't allow for cascading. Though it should still be available to the developer, so I think having both is a good idea.
But perhaps they should rely on a shared function to ensure they remain the same.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 6, 2018

Contributor

I didn't know anyone cared why I think it's weird :D actually I just mentioned it in case someone else felt the same.

The reason I think it's weird is that a player represents some abstract game/model related interface. Player's methods usually involve manipulating a player's properties in the context of the game. Camera and camerasetup is an abstraction around a view of the game playing field. Specifying a player when interacting with them only ever impacts the scope over which those views are manipulated.

In other words, I think of the built-in free-function camera API like:

  • camera free functions are tools for changing a view over the game terrain
  • camera free functions sometimes (always? does it matter?) take a player as a means for limiting that scope
  • player in the sense of the model as related to the game is otherwise not impacted

Anyway, if I really hated this idea, I would be shouting, but I don't, so I just say "seems weird"

public function player.panCameraToTimed(vec2 pos, real duration)
PanCameraToTimedForPlayer(this, pos.x, pos.y, duration)

/** Pans the camera depending on how far pos is from the current

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 4, 2018

Contributor

Is this safe in multiplayer?

This comment has been minimized.

@Ouguiya

Ouguiya Jun 4, 2018

Author Contributor

These are the functions that GUI uses, which basically do stuff for the local player (and nothing else). If they weren't safe in Multiplayer, practically any GUI-coded map that alters the camera would desync.

Edit: In case you are curious, this is the function used:

function PanCameraToTimedForPlayer takes player whichPlayer, real x, real y, real duration returns nothing
    if (GetLocalPlayer() == whichPlayer) then
        // Use only local code (no net traffic) within this block to avoid desyncs.
        call PanCameraToTimed(x, y, duration)
    endif
endfunction

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 4, 2018

Contributor

I'm somewhat confident that the function using SMARTPAN_THRESHOLD can cause a desync in multiplayer, and indeed, a number of GUI functions can cause desyncs.

This comment has been minimized.

@Frotty

Frotty Jun 4, 2018

Member

Is bj_SMARTPAN_TRESHOLD_PAN modified by gameplay settings or how do you explain using a constant causing desyncs?

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 5, 2018

Contributor

I don't know. I think it's a good strategy to rely on what lep/jassdoc says until proven otherwise. If those BJ functions aren't documented as breaking stuff then we're actually acting as good members of the community by using them, even if that means we find bugs

else if dist >= bj_SMARTPAN_TRESHOLD_PAN
panCameraToTimed(pos, duration)

public function player.setCinematicCamera(string cameraModelFile)

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 4, 2018

Contributor

Is string parameter right here? I guess yes, but maybe add some hotdoc.

Update Camera.wurst
- Added hotdoc for setCinematicCamera
- Overloaded "WithZ" functions to use vec3
- Added hotdoc for panCameraTo
@Frotty

Frotty approved these changes Jun 10, 2018

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jun 10, 2018

Looks good to me now @Cokemonkey11

@Cokemonkey11

This comment has been minimized.

Copy link
Contributor

Cokemonkey11 commented Jun 10, 2018

LGTM too

@Frotty Frotty merged commit a10e4a2 into wurstscript:master Jun 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment