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

Greenhouse not always working. #12

Closed Langly- closed 9 years ago

Langly- commented 9 years ago

Greenhouse seems to only be ignoring biome growth rates in biomes where something can grow. You can get faster rates for biomes a crop grows very slowly in, but it won't work at all in a biome something can not grow in. That is not ignoring biome, and something is odd.

Secondly, the config spreadsheet on txapu has been showing Dark Oak should now grow in mushroom biomes, that does not work.

ttk2 commented 9 years ago

this is the right place, thanks.

On Fri, May 1, 2015 at 1:38 PM Langly- notifications@github.com wrote:

Greenhouse seems to only be ignoring biome growth rates in biomes where something can grow. You can get faster rates for biomes a crop grows very slowly in, but it won't work at all in a biome something can not grow in. That is not ignoring biome, and something is odd.

Secondly, the config spreadsheet on txapu has been showing Dark Oak should now grow in mushroom biomes, that does not work.

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12.

WildWeazel commented 9 years ago

It does ignore biomes, because glowstone and biome modifiers are calculated independently. It simply replaces sunlight with a 75% modifier. The alternative would be to override the biome rate with the glowstone rate. It's a poorly named variable that isn't being used.

Langly- commented 9 years ago

The biome rate was being overridden in some cases, potato grow faster in plains with glowstone than they do in sunlight. Overriding biome rate would be the best and make the most sense, since that would be realistic as far as growing things go. And thanks.

On Fri, May 1, 2015 at 1:27 PM, Travis Christian notifications@github.com wrote:

It does ignore biomes, because glowstone and biome modifiers are calculated independently. It simply replaces sunlight with a 75% modifier. The alternative would be to override the biome rate with the glowstone rate. It's a poorly named variable that isn't being used.

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-98227193 .

ttk2 commented 9 years ago

whats needed to fix this?

On Sat, May 9, 2015 at 12:15 PM Langly- notifications@github.com wrote:

The biome rate was being overridden in some cases, potato grow faster in plains with glowstone than they do in sunlight. Overriding biome rate would be the best and make the most sense, since that would be realistic as far as growing things go. And thanks.

On Fri, May 1, 2015 at 1:27 PM, Travis Christian <notifications@github.com

wrote:

It does ignore biomes, because glowstone and biome modifiers are calculated independently. It simply replaces sunlight with a 75% modifier. The alternative would be to override the biome rate with the glowstone rate. It's a poorly named variable that isn't being used.

— Reply to this email directly or view it on GitHub < https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-98227193

.

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-100518786 .

WildWeazel commented 9 years ago

If you can, get some screenshots with debug mode and the growth rate message for both cases.

plebes commented 9 years ago

I would like to fix this problem, and I think I know what it is. I just want to make sure that the greenhouse feature is where crops adjacent to glowstone faces have the possibility of growing if configured to do so in the config.

Also, regarding the getRate() function in GrowthConfig, if a crop does not require sunlight to grow, should it still incur a penalty for not being at max sunlight level? (This is the case currently).

Once I know, I will commit.

Edit: Langly is correct in his observation. Potatos take 6 hours in plain under full sunlight, and 4 hours in the same spot with just glowstone.

ttk2 commented 9 years ago

yes that is how the greenhouse function should work as far as I know.

If the crop does not require sun, it should not get a penalty, although this might lead us to making some crops that don't currently require run require it if they slipped through the cracks due to this.

I hope that answers your quesitons, just ask if you need any help at all.

On Sat, May 30, 2015 at 2:43 AM plebes notifications@github.com wrote:

I would like to fix this problem, and I think I know what it is. I just want to make sure that the greenhouse feature is where crops adjacent to glowstone faces have the possibility of growing if configured to do so in the config.

Also, regarding the getRate() function in GrowthConfig, if a crop does not require sunlight to grow, should it still incur a penalty for not being at max sunlight level? (This is the case currently).

Once I know, I will commit.

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-107003066 .

WildWeazel commented 9 years ago

I suppose the way it should work is if glowstone is found, take the greater of 75% or the calculated sunlight rate. Not sure what the intention was for non-sunlight, but they should probably not grow any faster with more/full light.

benjajaja commented 9 years ago

@Langly- I will update http://txapu.com/rb.html to show growth times/percentage with glowstone too, mostly for usability, I'd like to know growth for greenhouse too ;)

Otherwise, I think the behaviour is correct - if something cannot grow at all in a biome, glowstone shouldn't work either, or you could just grow A LOT of things in most biomes. You could suddenly grow most crops and trees in an ocean biome!

ProgrammerDan commented 9 years ago

It'd be kind of neat to allow crop growth out of biome with glowstone, but that would be a pretty big feature break.

Imagine 1%-5% of "allowed" biome growth rate, but only in a greenhouse. It'd take a week to grow anything :)

Langly- commented 9 years ago

It would make sense for glowstone to work in other biomes though. If you built a a heated lit cavern in Antarctica you could still get stuff to grow. Growing things with glowstone gets costly, and would still limit what could be done in other biomes. It would also let ocean cities be a bit more feasible, yet still costly to grow crops in.

On Mon, Jun 8, 2015 at 8:38 AM, Benjamin Grosse notifications@github.com wrote:

@Langly- https://github.com/Langly- I will update http://txapu.com/rb.html to show growth times/percentage with glowstone too, mostly for usability, I'd like to know growth for greenhouse too ;)

Otherwise, I think the behaviour is correct - if something cannot grow at all in a biome, glowstone shouldn't work either, or you could just grow A LOT of things in most biomes. You could suddenly grow most crops and trees in an ocean biome!

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-110047218 .

ProgrammerDan commented 9 years ago

Maybe we should open a Discourse on this?

benjajaja commented 9 years ago

Go ahead, I will argue for LOWERING greenhouse efficiency >:)

ProgrammerDan commented 9 years ago

I'd agree with you! :P

Edit: and then turn around w/ Langly's argument hehe.

Langly- commented 9 years ago

Lower the efficiency, but allow it to work anywhere. Meaning limited but possible crops. That would work as a nice even balance.

On Fri, Jun 12, 2015 at 9:03 AM, Daniel Boston notifications@github.com wrote:

I'd agree with you! :P

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-111536274 .

benjajaja commented 9 years ago

@Langly- Then at once, location/biome would be pretty meaningless, and a massive amount of people that are using existing greenhouse ratios would be very upset?

ProgrammerDan commented 9 years ago

I'm thinking from a balance perspective, volume alone and general availability of glowstone (1DC was publicly traded by a single individual on Wednesday in the space of two hours with promise of more easily available) demands something like 25-50x less efficient in greenhouses, regardless of biome, but I'd expect like 25x less efficient in allowed biome, and 50x less efficient in "non-standard" biome.

Basically, you can grow crops "anywhere" in a greenhouse, but you'll need a farm 50x times as large as just grabbing a plot within biome, and you'll need enough glowstone to meet the adjacency requirement for that whole 50x large field.

Might be missing something, but sounds like a good tradeoff.

On Fri, Jun 12, 2015 at 12:49 PM, Langly- notifications@github.com wrote:

Lower the efficiency, but allow it to work anywhere. Meaning limited but possible crops. That would work as a nice even balance.

On Fri, Jun 12, 2015 at 9:03 AM, Daniel Boston notifications@github.com wrote:

I'd agree with you! :P

— Reply to this email directly or view it on GitHub < https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-111536274

.

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-111553964 .

ProgrammerDan commented 9 years ago

@gipsy-king Let them eat cake! (still, valid objection)

benjajaja commented 9 years ago

People will build anything if they can get around dealing (especially economically) with other people. No matter how large they must make their farm or what it costs, they will build it if it means they don't have to travel or trade with others. Also, we're getting compactors soon unless I'm mistaken, so that's one more point against this.

Greenhouse defeats the purpose of RB at the current rate. But I wouldn't really argue for lowering the rate, because that would just suck for all the players who have already built large greenhouse farms. Maybe reduce it from 75% to 60% or something, with the introduction of compactors being the excuse.

The enabling of greenhouse in ANY biome, IF done, should at most be at like 1%? So that you can grow for decorative purposes only, nothing else.

ProgrammerDan commented 9 years ago

The enabling of greenhouse in ANY biome, IF done, should at most be at like 1%? So that you can grow for decorative purposes only, nothing else.

I think we're well into the territory of wishful thinking, because you've hit on the core issue -- greenhouse defeating the purpose of having value within specific biomes if we open that floodgate at all. Most suggestions on rebalance completely miss the total focus and absolutely maniacal devotion our playerbase has to subvert the goals and go lone wolf or game the system.

Even as I wrote about 50x reduction in output (like .5% of biome-growth) I was envisioning construction of a MASSIVE 512x512x60 multi-level farm, employing armies of bots to harvest each level, built into the ocean. At that size it'd more than make up for the biome growth reduction, although at great cost.

WildWeazel commented 9 years ago

In the original Civtest where this was introduced we did allow glowstone to override biome settings, and it was evident that if it was powerful enough to be worth using at industrial scale, it would make biomes irrelevant and defeat the purpose of the plugin.

ProgrammerDan commented 9 years ago

@plebes We've hijacked your thread with nonsense. Do you need any assist on moving this patch forward?

plebes commented 9 years ago

Hi Dan,

I re-wrote the getRate function with a fix to the logic and rate calculation. However, I didn't issue a pull request because I would like to test the changes on a local build of the realistic biomes plugin before the pull request.

I set up a bukkit server, and have written and tested my own plugins (a change to the anvil). However, I've not been able to do so with realistic biomes.

If you, or someone else, can help me get realistic biomes running with the sql server locally, then I can continue the bug fixes.

My goal was to contribute to the civcraft server by fixing the bugs, as it is fun, and useful. I picked realistic biomes because no one had been working on the bugs, and I found some quite easily.

If I get a local build of the plugin working, then I will fix the bugs and add whatever else is wanted.

Thanks.

benjajaja commented 9 years ago

@plebes you just need both a database and user called "tekkit", just give the user all privileges.

Regarding getRate, there is no bug as far as I understand. It is too complex, yes. A rewrite is tricky, because the server really depends on the exact implementation. I wouldn't rewrite this one without writing a test :open_mouth:

ttk2 commented 9 years ago

RB in its current state is not doing as much as it perhaps should to achieve the goals of biome specific production, especially since biomes are small enough to allow easy travel between them and the setup of multiple farms, not totally sure if these concerns are super relevent, we don't exactly want to make things worse but at the same time they can't get too much worse and our major concern should be if this interferes with our ability to fix things.

Plebs, thanks for the coding work! Sorry you accidentally chose a contentious issue to start with, it happens, at the very least having the feature working again will be appreciated even if it remains off. Just let me know if you need anything.

On Sat, Jun 13, 2015 at 4:07 AM Benjamin Grosse notifications@github.com wrote:

@plebes https://github.com/plebes you just need both a database and user called "tekkit", just give the user all privileges.

Regarding getRate, there is no bug as far as I understand. It is too complex, yes. A rewrite is tricky, because the server really depends on the exact implementation. I wouldn't rewrite this one without writing a test [image: :open_mouth:]

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-111689123 .

plebes commented 9 years ago

@gipsy-king Thank you for the information. I thought that I had to run a script to set up all of the tables first. If it is created automatically, then that is great.

Also, there does seem to be a logic error in the getRate function. All crops, even if they do not require sunlight, have a penalty if they are not in full sunlight. Look at the conditional statement on line 219. It would seem that it should be nested inside the conditional for crops that need sunlight on 215.

Also, If you go into the plains biome (for instance), you will see that crops (potato) grow faster with the glowstone greenhouse feature than the same crop in full sunlight, which is what I was asking about previously.

@ttk2 I will try to get the database working with the realistic biomes plugin and then help out more.

In addition, when sharding is incorporated, couldn't you do some new feature such as having biomes in the shards having independent growing weights that can be cycled? If you have 7 shards, then you can cycle a "shard weight" amongst those shards. So sometimes one shard has the best growth, and the next another. You can cycle it each day or one a week. That might encourage trade as well since one groups shard may have very poor growth for the week, while their allies has the best growth. Basically, it allows you to simulate sunlight moving over a planet and get around the minecraft universal time for all chunks.

benjajaja commented 9 years ago

@plebes I didn't write RB but I did make http://txapu.com/rb.html so I will try to give you an explanation on those lines of code:

The only crops that don't require sunlight but do have a notFullSunlightMultiplier are nether warts, which is the only crop that has that set as boost, not penalty! notFullSunlightMultiplier default value is 1, so if not set it does nothing.

This is just one example of why you are obvisouly correct about it all being an ungodly mess, both for coders, config-file editors, and end users that try to make sense of it.

While working on it today, I was contemplating throwing it away and rewriting everything, but that would of course require a huge deal of testing. What do you think about waiting for me to implement persistence for "fruitful" crops (crops that spawn blocks: melon, pumpkin, cactus, sugarcane, mushroom and sapling), and then try a complete rewrite? I know it may sound like a waste of my time, but it would help a lot if we knew the real requirements and pitfalls of these fruitful crops.

plebes commented 9 years ago

@gipsy-king It would be fun from a coding perspective to do a full rewrite, but I don't think it would be best for Civcraft itself. In my opinion, if we contribute, it would be best to fix the bugs currently known, which are very few, than to write an entirely new version of this plugin. However, if you have a design that has improved performance in terms of structure or algorithms used, then a rewrite would be good. I only speak for my own opinion in that I want to contribute by fixing the bugs, and improving the code in terms of safety at the same time. To be honest, my time is limited, so this is the best way to contribute for me.

If you do begin a new version of RB, then please do let me know about your repo.

Thanks

benjajaja commented 9 years ago

@plebes no, it was just an idea.

As I said I'm working on persisting these new crops right now, I haven't touched GrowthConfig.java and I don't think I will have to, because I don't think it is buggy per se - it's just counterintuitive and extremely difficult to make out. I think if you changed getRate, you would have to adapt the config too, so that it all ends up understandable without going through the code.

plebes commented 9 years ago

@gipsy-king Currently crops like potato can grow faster in the plains with glowstone than with full sunlight, which is what I think they said to change up above. If you can change that, then the get rate function is confusingly coded, but results will be fine. I think I will stay away from get rate right now since you seem to know it better than me.

benjajaja commented 9 years ago

potato grows faster in plains with glowstone - sounds correct, without glowstone it's 0.5 efficiency (it's 1.0 in mountains), with glowstone you should get 0.75 efficiency which would be 6h vs 4h30min in plains I think from what I can gather at http://txapu.com/rb.html

ttk2 commented 9 years ago

Feel free to refactor as much as you need to and try to clean things up. Unless you go into rewrites with very strict rules about what you are out to fix it usually does not help as much as people think it would.

On Sat, Jun 13, 2015, 3:03 PM plebes notifications@github.com wrote:

@gipsy-king https://github.com/gipsy-king Currently crops like potato can grow faster in the plains with a glowstone than with full sunlight, which is what I think they said to change up above. If you can change than, then the get rate function is confusingly coded, but results will be fine. I think I will stay away from get rate right now since you seem to know it better than me.

— Reply to this email directly or view it on GitHub https://github.com/Civcraft/RealisticBiomes/issues/12#issuecomment-111743847 .

benjajaja commented 9 years ago

@Langly- @plebes can we close this?

benjajaja commented 9 years ago

Closing since greenhouse appears to be working as described in the wiki. @Langly- if the dark oak issue still isn't solved, reopen this issue.