0001-Merged-sapling-onto-tree-species-fix.patch
0030-Fixed-issues-with-setting-tree-species.patch
[SPIGOT-1389] Tree species is not set correctly for certain block types by the Tree class Created: 01/Jan/16 Updated: 07/Feb/16 Resolved: 05/Jan/16 |
|
| Status: | Resolved |
| Project: | Spigot |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Minor |
| Reporter: | Ryan Bennitt | Assignee: | md_5 |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Environment: |
All environments |
||
| Attachments: |
0001-Merged-sapling-onto-tree-species-fix.patch
0030-Fixed-issues-with-setting-tree-species.patch
|
| Plugin: | Bukkit |
| Description |
|
The Tree class is set to be used by various blocks for setting the TreeSpecies in the MaterialData, however it doesn't set it correctly for block types. The WoodenStep class also has this issue. They make the assumption that only the first two bits are used for TreeSpecies when for WOOD, SAPLING, WOOD_STEP, WOOD_DOUBLE_STEP the values 0-5 are used (first three bits). Also LEAVES_2 and LOG_2 uses the first bit to represent ACACIA or DARK_OAK, which has not yet been implemented. |
| Comments |
| Comment by Ryan Bennitt [ 07/Feb/16 ] |
|
Attached patch merging Niklas Merkelt's Sapling class, plus unit test, plus formatting. |
| Comment by Ryan Bennitt [ 10/Jan/16 ] |
|
I don't see this on the 1.9 branch? I can see someone else has a pull request for commit e9461497b5c to add fixes to LEAVES_2 and LOG_2. They have done the same fixes for these two block types though, it fixes the constructor choosing the correct LOG/LOG_2 and LEAVES/LEAVES_2, and fixes the translation between block data and TreeSpecies for LOG_2/LEAVES_2 in set/getSpecies. |
| Comment by Ryan Bennitt [ 10/Jan/16 ] |
|
I have access! Created pull request for master. |
| Comment by md_5 [ 05/Jan/16 ] |
|
I've pulled this to the 1.9 branch. |
| Comment by Ryan Bennitt [ 03/Jan/16 ] |
|
I've reviewed my code from a backwards compatibility perspective. I've not removed any constructors. I've moved the set/get species method to the super class, but it is still available for use for all subclasses. All the new MaterialData classes have an (int,byte) constructor, which is what the Material class uses to instantiate them. Anyone using the Tree class in a plugin to represent LEAVES, WOOD etc, is still able to do so, but I guess at the moment they might be hacking the data bits using the existing broken implementation of setSpecies plus a setDirection call to get around the bug, so they may be surprised when setSpecies starts working correctly for these block types. I think it should still work OK for them as everything is still changing the underlying data bits on the block, which is ultimately all that matters. If anyone is using WoodenStep for a WOOD_DOUBLE_STEP, they should not be attempting to invert it as it is not invertible, but they can still create an instance of WoodenStep and pass it the double step material so they should be fine also. I did notice I hadn't implemented the decaying flag in the Leaves class, so I have added that in. This would only have affected new implementations using this class though. Added the decaying flag to the existing unit tests. |
| Comment by md_5 [ 03/Jan/16 ] |
|
Bit concerned about changing the MaterialData for those materials. Won't it break things that use it already? |
| Comment by md_5 [ 03/Jan/16 ] |
|
Ok thanks, I'll get these patches sorted soon then. |
| Comment by Ryan Bennitt [ 03/Jan/16 ] |
|
Yeah, I found the CLA after I'd created these tickets and submitted it Friday. |
| Comment by md_5 [ 03/Jan/16 ] |
|
As per other ticket, please fill in the CLA |
| Comment by Ryan Bennitt [ 01/Jan/16 ] |
|
Attached patch fixing refactoring the tree species code. (Not yet got access to push changes...) Created Wood class as parent of Tree/Leaves/WoodenStep which calculates material data bits based on block type. |