Conversation
TwistedAsylumMC
left a comment
TwistedAsylumMC
left a comment
There was a problem hiding this comment.
Looks good! Just a few things mostly with docs. Is State an appropriate name for the field & methods considering it's actually the next state?
|
|
||
| // ViewEntityAnimation ... | ||
| func (s *Session) ViewEntityAnimation(e world.Entity, animationName string) { | ||
| func (s *Session) ViewEntityAnimation(e world.Entity, animation animation.Animation) { |
There was a problem hiding this comment.
I'd just rename this variable to a to avoid conflicting with the package, same thing in the interface yupe in viewer.go
| return Animation{ | ||
| name: animationName, | ||
| state: "", | ||
| controller: "", | ||
| stopCondition: "", | ||
| } |
There was a problem hiding this comment.
This can just be Animation{name: name}
| // Animation represents an animation & controller that may be attached to an entity. | ||
| // Animations and controllers must be defined in a resource pack |
There was a problem hiding this comment.
Something like this would sound better
// Animation represents an animation that may be played on an entity from an active resource pack on
// the client.| // WithController sets the controller with the specified state. | ||
| // The controller must be added in a resource pack |
There was a problem hiding this comment.
// WithController returns a copy of the Animation with the provided animation controller. The
// controller must be defined in a resource pack for it to work.| // Controller returns the name of the controller being used. Controller returns an empty string if | ||
| // no controller was previously set |
There was a problem hiding this comment.
Don't think it's necessary to mention the empty string here, also purely just for OCD reasons could you move Controller(), State() and StopCondition() methods to be before their respective With...() methods?
| return a.controller | ||
| } | ||
|
|
||
| // WithState sets the state to transition to as defined in the controller. |
There was a problem hiding this comment.
// WithState returns a copy of the Animation with the provided next state.| // State returns the current state being played. State returns an empty string if | ||
| // no controller was previously set |
There was a problem hiding this comment.
Same thing, no point mentioning empty string. This is also incorrect since it's not the state that is currently being played. Something like this may be better:
// State returns the name of the next state in the animation controller.| return a.state | ||
| } | ||
|
|
||
| // WithStopCondition takes the molang expression and stops the animation if the query passes. |
There was a problem hiding this comment.
// WithStopCondition returns a copy of the Animation with the provided stop condition. This condition is
// usually a Molang query which is constantly compared against until the animation has stopped.| // StopCondition returns the stop condition. StopCondition returns an empty string if | ||
| // no molang expression was set |
There was a problem hiding this comment.
Same thing, no point mentioning empty string
| // AnimateEntity will start an animation for the specified entity. Viewers that are viewing the entity will be | ||
| // played the animation. | ||
| func (w *World) AnimateEntity(e Entity, animation animation.Animation) { |
There was a problem hiding this comment.
PlayEntityAnimation might be a better name to fit with PlaySound etc.
|
Closing in favour of #940 |
3 participants
The documentation probably needs improvement. Also tie-in pr for the wiki
Tests
Test resource pack: EntityAnimationTest.zip