EarendelDevelopers / factorio-mods

This is a public repository for tracking issues with Earendel's factorio mods.
19 stars 3 forks source link

Alien Biomes assumes rubber tree autoplace exists #250

Closed Deadlock989 closed 1 year ago

Deadlock989 commented 1 year ago

data-updates in Alien Biomes makes an adjustment to the autoplace of any tree it finds with substring "rubber" in the name - I'm assuming this was to improve compatibility with IR2, thanks for that.

In IR3 (in development) there are some additional tree prototypes which are variants without an autoplace, for a specific purpose. One of them needs to have "rubber" in the name. The data-updates code in Alien Biomes does not test that tree.autoplace exists before trying to index it, so loading halts with an error. Easy fix is to test that tree.autoplace isn't nil.

There are many other conflicts between IR3 and the AB/AAI/SE collection but this one is short work.

Thanks

InappropriatePenguin commented 1 year ago

Should be part of the next release. Can you let us know when the IR3 release is imminent?

InappropriatePenguin commented 1 year ago

Were there other issues specific to AB by the way? AB doesn't get updated frequently so I'd rather make all the compatibility changes at the same time.

AAI/SE compatibility can wait until after release, since issues are likely to be a lot more complex anyway.

Deadlock989 commented 1 year ago

To be honest I'm not very familiar with what AB does and was unaware that it had been modified to work at all, the last I heard AB was just removing the rubber tree autoplace if and only if your mod combo ended up with AB loading after IR. I can see that there is now an additional tile restriction being put on IR rubber trees to just one tile type ("grass"), but five minutes looking at random seed map previews, for some seeds they are spawning in vast numbers compared to the IR settings, while on other seeds there are none or almost none. I don't really know if that's a problem or not but ideally you should be seeing at least some on a map preview (the map colour is slightly different to other trees, more yellow-green). The effect of their usual autoplace is that it's possible for them to occur in small to medium forests only in the very warmest and wettest areas of a vanilla map, while everywhere else you mostly only get fairly small strings of them around water, i.e. at low elevation. To be honest I find that dealing with the noise/autoplace system is like passing notes handwritten in crayon under a door to an elder abomination from an eldritch dimension and expecting to get something meaningful back.

But with that change at least both mods can load at once, thanks for doing that.

InappropriatePenguin commented 1 year ago

Yeah, noise functions/autoplace rules are quite opaque to me since I mostly dabble with control stage code. Fortunately, Earendel seems to know their way around them, so I can ask them if there's a simple way to mimic the placement rules you're describing with AB.

I don't know if it's better for AB to dictate that placement in data-updates or for IR to do it. There's a check in prototypes/entity/trees.lua already that protects trees with "rubber" in their name from having their autoplace tables deleted. The argument would be that it's better that IR dictate what biome/tile restrictions its rubber trees should have when AB is loaded, if any.

And you're welcome ;) The change you requested was more akin to a bugfix than a feature request anyway so it's a no-brainer to make.

EarendelGames commented 1 year ago

dealing with the noise/autoplace system is like passing notes handwritten in crayon under a door to an elder abomination from an eldritch dimension

Yeah that sounds about right.

The effect of their usual autoplace is that it's possible for them to occur in small to medium forests only in the very warmest and wettest areas of a vanilla map, while everywhere else you mostly only get fairly small strings of them around water, i.e. at low elevation.

For the autoplace expression that's pretty good in general, the only problem area is the temperature extremes. They probably shouldn't be on the volcanic tiles becuase there are no normal trees there. I'm not sure about the frozen terrain, it's usually just the cold adapted trees, but the frozen areas can be quite large so they might need to be allowed there for balance reasons.

Deadlock989 commented 1 year ago

I don't know if it's better for AB to dictate that placement in data-updates or for IR to do it. There's a check in prototypes/entity/trees.lua already that protects trees with "rubber" in their name from having their autoplace tables deleted. The argument would be that it's better that IR dictate what biome/tile restrictions its rubber trees should have when AB is loaded, if any.

The problem there is that I have no idea of where to start with that and will most likely not find time to ever get around to it ...

The effect of their usual autoplace is that it's possible for them to occur in small to medium forests only in the very warmest and wettest areas of a vanilla map, while everywhere else you mostly only get fairly small strings of them around water, i.e. at low elevation.

For the autoplace expression that's pretty good in general, the only problem area is the temperature extremes. They probably shouldn't be on the volcanic tiles becuase there are no normal trees there. I'm not sure about the frozen terrain, it's usually just the cold adapted trees, but the frozen areas can be quite large so they might need to be allowed there for balance reasons.

It's not critical and probably not worth spending much/any time on. The main consideration is that there are not zero rubber trees within a five minute wheeled vehicle drive from coordinate origin - I am absolutely fine with people having to build a vehicle and explore to get some, but not for them to spend time doing that fruitlessly. In IR2 they only need (I think) one tree to bootstrap rubber wood duplication. In IR3 it is more involved than that - they can technically bootstrap a rubber tree plantation after finding just three trees, but that would take an eternity to build up speed, really they want to find at least a small cluster of them. If, however, there are masses of trees everywhere, then it becomes trivial, but that is not a problem in the same way that finding none of them is.

I recommend removing the "grass tiles only" restriction and if you want to take the time and think it's thematically important, adding a couple of blacklisted tiles like the volcanic or frozen tiles, but I don't know much about that to be honest.

InappropriatePenguin commented 1 year ago

I recommend removing the "grass tiles only" restriction and if you want to take the time and think it's thematically important, adding a couple of blacklisted tiles like the volcanic or frozen tiles, but I don't know much about that to be honest.

That actually turned out to be simpler than I expected. Replacing line 3 in AB's data-updates.lua with the following seems to do the trick. I didn't test different map seeds extensively, but at a minimum, it should be less restrictive than it was before.

tree.autoplace.tile_restriction = alien_biomes.list_tiles(
      alien_biomes.exclude_tags(alien_biomes.all_tiles(), {"volcanic", "frozen"}))
Deadlock989 commented 1 year ago

Thanks for doing that.