Fixed issue #52 #53
Fixed issue #52 #53
Conversation
|
sweet, repeating my question from the other thread, is this good to merge? |
|
Yes it is. |
|
cool, its on civtest |
|
@soerxpso looking at the commit a bit: https://github.com/Civcraft/RealisticBiomes/pull/53/files#diff-45dadbac82aadd0a4b4d509bc3e57d7bR97 Are there any negatives to "dropping out" of the interaction event before performing the grow and persist method? E.g. with this change you can't force the crop to render at the accurate growth value more than once per second. For large fields or farms, this means you can't rapidly update them. Other than placement, I think the code looks sound. |
|
@ProgrammerDan It looks like you're correct. Doesn't it also work to just reload the chunk by relogging if you need the crops to re-render though? In any case you can change the placement if you think it needs changed. Just a simple ctrl+c ctrl+v. |
| Block block = event.getClickedBlock(); | ||
|
|
||
| if (((List<ItemStack>)block.getDrops()).get(0).getType() == event.getMaterial()) { |
There was a problem hiding this comment.
Surely this cast is unsafe and could be changed to something like block.getDrops().iterator().next()?
Edit: plus it probably needs a null-check anyhow. This should be throwing NPEs right now when breaking blocks that don't give drops.
There was a problem hiding this comment.
According to the bukkit docs, it is a safe cast. And don't call me Shirley.
|
What is our total unique player count? Unless over 10k in a single day i have no concerns. @Soerxpso no, iirc the chunk load doesn't update all the crops, hence the hit command to force it to do so. As for who should make the edit, unfortunately this was prematurely merged before others could review it, hence yes someone else might have to fix it. Traditionally, however, on reviews the original poster of the pull takes ownership of corrections highlighted by reviewers. Asking them to fix it is in bad form (generally) unless the fix is unclear or outside the ability of the original poster. |
|
total unique? 40k range |
|
Ever had a day where all 40k visited? I'd guess you get a sample of about 1k each day? |
|
What about the NPE? I'm gonna revert @soerxpso and handle null and remove Ever had a day where all 40k visited? I'd guess you get a sample of about — |
|
getting logins per day is easy, uniques per day will require a little more On Fri, Aug 14, 2015 at 11:06 AM, Benjamin Grosse notifications@github.com
|
|
@gipsy-king VERY good catch on that NPE, totally missed it. Clearly wasn't thinking it through when reviewing. Edit: Is there a block that doesn't give drops? It's context-dependent, right? If you break stone without a tool, there are no drops, otherwise, cobblestone. Definitely worth the null check, though. Yeah, revert and repair is best option. Also, probably a good idea to give the concurrent hashmap an initial capacity of 1000 so there's very little backing-store-churn. @ttk2 Was more a framing question; exact numbers not needed. 40k is still small enough that even if every single person logged in before restart, I'd not be overly worried about a UUID/Long map using up too much memory. |
|
@ProgrammerDan once sharding is a thing we need to be more memory cautious, but for the time being we have 64gb of ram and MC won't use more than 10ish on its own because GC starts to take too long. |
|
@ttk2 Do we have some clear idea the hardware specs per shard? If we do, I could take that into account while working on mods, and pull together some impact analysis kind of stuff |
|
we can buy what we need, but ideally we would run three or four shards on a machine like the current one (3.7 ghz hyperthreaded quad core with 64gb ddr3 and 128bg ssd) |
|
Gives us a lot to work with, great. Might need to do some hardcore "overall" analysis esp. with the caching changes for citadel and other in-memory tracking. |
|
frankly if you need ssh you can have it. |
The chat is no longer spammed with messages from RealisticBiomes when you're harvesting a crop using the same type of crop in your hand.