Add noteblocks and instruments #133
Conversation
|
If you are concerned changing the World trait will break some other application you could add a default implementation to the new function you are adding. |
|
The implementation for the direct backend is a bit hacky, but it was the best thing I could come up with without rewriting major parts and thereby breaking most other PRs. There is also a bit of logic duplication for noteblocks and their sounds and I am not entirely sure where such utility functions should belong. |
There was a problem hiding this comment.
Out of curiosity, do you know if this would negatively impact performance after #129?
Paul1365972
force-pushed
the
noteblocks
branch
2 times, most recently
from
53c79d7
to
3c113bc
Compare
|
Rebased and benchmarked against current master. Performance is suprisingly enough exacltly the same. |
crates/core/src/redstone/mod.rs
Outdated
| powered: should_be_powered, | ||
| }; | ||
|
|
||
| // TODO: There is a weird double activation bug, so we need to check if block is not already powered |
There was a problem hiding this comment.
What is going on here? I'm not sure what you mean by double activation bug. Is it simply that the block updates multiple times? That's no guarantee it'll update only once after any change in surrounding state, or in response to a state change in general.
We already have block and powered. That state shouldn't have changed by this point, so I don't see why we need to retrieve and check it again.
There was a problem hiding this comment.
The block that is passed to the update method can definitely be outdated.
You are right that in nearly all cases the block is up-to-date as it fetched right before the update call, however in the update call in turbo::breadth_first_walk an old block instance is sometimes used.
There was a problem hiding this comment.
I looked into it a bit more and the comment on line 303 of turbo.rs basically explains it.
This only works because updating any other block than a wire will never change the state of the block. If that changes in the future, the cached state will need to be updated
Noteblocks break that assumption as they do change state on update.
All other blocks are correctly unaffected by "double updates", either because they schedule a tick (repeaters, torches etc.) or because only the final state matters (trapdoors etc.).
Paul1365972
force-pushed
the
noteblocks
branch
2 times, most recently
from
b4c7ba2
to
55ab46a
Compare
Also added at least one block necessary for each instrument Note: Compiled noteblocks do not work yet
|
Fixed all conflicts with the refactored redpiler and also updated the logic where necessary. |
This PR adds the noteblock and all blocks necessary to at play each instrument.
Not all blocks are implemented like logs or planks, but substibutes exist for each instrument (see: https://minecraft.wiki/w/Note_Block#Instruments).
This is still WIP for now and does not work with the redpiler yet.
Also small warning: The World trait now has a play_sound function, this will probably be a breaking change some software I don't know about. However adding it to the World trait still seemed like the best choice architectually.