search

Civcraft / RealisticBiomes

Do not open issues here; open them on the maintained fork @ DevotedMC
https://github.com/DevotedMC/RealisticBiomes
7 stars 14 forks source link

Fixed issue #52 #53

Closed sepiatonal closed 8 years ago

sepiatonal commented 8 years ago

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.

ttk2 commented 8 years ago

sweet, repeating my question from the other thread, is this good to merge?

sepiatonal commented 8 years ago

Yes it is.

ttk2 commented 8 years ago

cool, its on civtest

ProgrammerDan commented 8 years ago

@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.

sepiatonal commented 8 years ago

@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.

ProgrammerDan commented 8 years ago

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.

ttk2 commented 8 years ago

total unique? 40k range

ProgrammerDan commented 8 years ago

Ever had a day where all 40k visited? I'd guess you get a sample of about 1k each day?

benjajaja commented 8 years ago

What about the NPE? I'm gonna revert @soerxpso and handle null and remove unsafe cast. El 14/8/2015 4:50 p. m., "Daniel Boston" notifications@github.com escribió:

Ever had a day where all 40k visited? I'd guess you get a sample of about 1k each day?

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/pull/53#issuecomment-131133540 .

ttk2 commented 8 years ago

getting logins per day is easy, uniques per day will require a little more work and log fu.

On Fri, Aug 14, 2015 at 11:06 AM, Benjamin Grosse notifications@github.com wrote:

What about the NPE? I'm gonna revert @soerxpso and handle null and remove unsafe cast. El 14/8/2015 4:50 p. m., "Daniel Boston" notifications@github.com escribió:

Ever had a day where all 40k visited? I'd guess you get a sample of about 1k each day?

— Reply to this email directly or view it on GitHub < https://github.com/Civcraft/RealisticBiomes/pull/53#issuecomment-131133540

.

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/pull/53#issuecomment-131140208 .

ProgrammerDan commented 8 years ago

@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.

ttk2 commented 8 years ago

@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.

ProgrammerDan commented 8 years ago

@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

ttk2 commented 8 years ago

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)

ProgrammerDan commented 8 years ago

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.

ttk2 commented 8 years ago

frankly if you need ssh you can have it.