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

Added more Effect functionality #87

Merged
merged 16 commits into from Jun 15, 2018

Conversation

Projects
None yet
3 participants
@eGlint
Copy link
Contributor

eGlint commented Jun 11, 2018

  • Optimized setPos(vec2) and setPos(vec3).
  • The yaw, pitch, roll of special effects can now use angle tuples.
  • The color of special effects can now use color and colorA tuples.
  • Implemented addEffect to destructables and widgets.
  • Created EffectUtils. It contains the EffectEx class which can retrieve the model name, attachment, yaw, pitch, roll, scale, etc. of the special effect. Something that the ordinary Effect package cannot do. It does not support BlzSetSpecialEffectTime since EffectEx cannot track if the special effect is destroyed.

eGlint added some commits Jun 7, 2018

Optimized setPos, added new functions
Optimized setPos(vec2) and setPos(vec3) by directly using the native function.
Added setColor. It can now use color and colorA tuples
Added setOrientation, setYaw, setPitch, and setRoll. It can now use angle tuples.
Create EffectUtils
Added EffectEx class, it contains data like yaw, pitch, roll, etc. that is not available in the default Effect functions. 
Does not support BlzSetSpecialEffectTime as EffectEx cannot track if the special effect is destroyed by this function.

@eGlint eGlint changed the title Added more special effect functionality Added more Effect functionality Jun 11, 2018

eGlint added some commits Jun 11, 2018

@Cokemonkey11

This comment has been minimized.

Copy link
Contributor

Cokemonkey11 commented Jun 11, 2018

Looks pretty good to me. Maybe squash?

@eGlint

This comment has been minimized.

Copy link
Contributor Author

eGlint commented Jun 11, 2018

I apologize, I'm not used to git. How do I squash my commits?

@Cokemonkey11

This comment has been minimized.

Copy link
Contributor

Cokemonkey11 commented Jun 11, 2018

If you don't know how, it isn't necessary - Frotty can squash on merge.

(But if you google it, you can learn to do it manually)

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jun 11, 2018

good stuff, but build is failing. Is it another overloading issue?

@eGlint

This comment has been minimized.

Copy link
Contributor Author

eGlint commented Jun 12, 2018

Wow, it now works! Thank you!

About the addEffect function for the Unit and Destructable packages, I removed it and placed it into the Widget package instead. It only broke one package in the standard library which is the Fx package. The error occurred due the usage of unit.addEffect and it can be easily fixed by importing the Widget package.

From my tests, it only breaks from packages that uses import NoWurst and unit.addEffect/destructable.addEffect. Like what I did in the Fx package, it can be easily fixed by importing the Widget package..

It leaves me a question: should we remove the addEffect function from the Unit and Destructable packages? Keep it? Deprecate it?

@@ -68,7 +68,7 @@ public function destructable.setOccluderHeight(real height)

public function destructable.getName() returns string
return GetDestructableName(this)

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member

shouldnt be here

BlzSetSpecialEffectColor(this, color.red, color.green, color.blue)
BlzSetSpecialEffectAlpha(this, color.alpha)

public function effect.setOrientation(angle yaw, angle pitch, angle roll)

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member

IIRC with latest patch the yaw/pitch/roll functions still have issues and might produce unexpected behaviour. Ideally add some hotdoc explaining what it does (rotating around which axis, whether it works or not)

This comment has been minimized.

@eGlint

eGlint Jun 13, 2018

Author Contributor

I did a test with the yaw/pitch/roll. Based on my test, BlzSetSpecialEffectPitch is the only function having issues when it reaches around 90 to 270 degrees yet the pitch in BlzSetSpecialEffectOrientation seems to work fine. Also I found out that the yaw and roll are inverted. (x/z instead of z/x)

Should we keep BlzSetSpecialEffectPitch for setPitch and add a warning? Or replace it with BlzSetSpecialEffectOrientation until the problem is fixed by Blizzard?

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 13, 2018

Contributor

Better set a warning I think

@@ -26,9 +26,6 @@ public function createUnitZ(player p, int unitId, vec3 pos, angle facing) return
public function unit.addAbility(int abil) returns boolean
return UnitAddAbility(this, abil)

public function unit.addEffect(string fx, string attachment) returns effect

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member

Before making breaking changes like this, deprecate the function.

this.attachment = attachment

private function initialze()
yaw = angle(0)

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member

these should be merged with the declarations, making this function superfluous

import Colors
import Player

public class EffectEx

This comment has been minimized.

@Frotty

Frotty Jun 13, 2018

Member

I'm not a fan of the name 'Ex' and the package name, and, this should be another PR.
Please keep PRs as short as possible and encapsulated, for faster review and merge.

This comment has been minimized.

@eGlint

eGlint Jun 13, 2018

Author Contributor

Sure, I'll keep that in mind and I'll remove this package in this PR.

public function effect.setYaw(angle yaw)
BlzSetSpecialEffectYaw(this, yaw.radians())

/**
Change the y-axis of the special effect.
Warning: As of 1.29.2, setPitch have issues when reaching the values 1.57-4.7 radians or 90-270 degrees. It is recommended to use setOrientation.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 14, 2018

Contributor

Is it documented in lep/jassdoc?

This comment has been minimized.

@eGlint

eGlint Jun 15, 2018

Author Contributor

I do not know about lep/jassdoc, but I did my research. I looked around lep/jassdoc git along with the forks and found nothing.

Also, I just posted a detailed bug report about this issue in Hive's Compiled List of 1.29 Bugs & Issues.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jun 15, 2018

Contributor

If it isn't already in lep/jassdoc then I would aks you to please contribute - https://github.com/lep/jassdoc/blob/master/effects.j

@lep fyi

@Frotty

Frotty approved these changes Jun 15, 2018

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jun 15, 2018

nice, thanks 🍬

@Frotty Frotty merged commit 32eb889 into wurstscript:master Jun 15, 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