Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reenable infiniteSky #5029

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Conversation

NicksWorld
Copy link

@NicksWorld NicksWorld commented Nov 16, 2024

This should allow infiniteSky to generate usable tiles in newer DF versions. The bug involving cave-ins removing z-levels appears to still be present, but to my knowledge the old workaround of saving/loading should still work.

As a allocating a new df::map_block and adding it to the map seemed to be the solution, there is a possibility of some data being lost for new tiles. For now the following data should be preserved:

  • Tiles receive light
  • Tiles are marked as outdoors
  • Standard and current temperature are inherited from map_block below

Fixes: #2606
Fixes: #3835

plugins/infiniteSky.cpp Outdated Show resolved Hide resolved
@myk002
Copy link
Member

myk002 commented Nov 22, 2024

Ok, so what you're saying is that #254 is still an issue, but #2606 (and #3835) should be fixed, right?

If so, could you add the text:

Fixes: #2606
Fixes: #3835

somewhere in the PR description? That will link this PR to those issues and close them when we merge this PR

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a line to the New Tools section in docs/changelog.txt? It should look like this: https://github.com/DFHack/dfhack/blob/develop/docs/changelog.txt#L199

Also, please edit docs/plugins/infiniteSky.rst and replace the dfhack-tool attributes with these lines:

    :summary: Automatically allocate new z-levels of sky.
    :tags: fort auto design map

Finally, in a separate PR, please feel free to add your name to docs/about/Authors.rst!

It looks like infiniteSky needs to be brought up to code standards before it is included in a release. For example, it needs to persist its enabled state and restore it when the fort is reloaded. It also needs to be registered with the control panel so it shows up for players in the tool list. None of this is particularly difficult (and there are good examples to copy/follow), but it will take time and may be more than you want to tackle at this point. If you'd like to dig deeper, I can walk you through it. Otherwise, I can do a pass after this PR is merged.

It also looks like some of the existing logic is incorrect. The "new construction scanning" in plugin_onupdate doesn't actually scan new constructions. New constructions are inserted sorted, not appended to the end of the vector. Therefore, infiniteSky may not add levels when it should. (it would work if the z coordinate were the primary sorting value, but for some reason, the x coordinate is the primary sorting value. why? why?)

plugins/infiniteSky.cpp Outdated Show resolved Hide resolved
@myk002
Copy link
Member

myk002 commented Nov 22, 2024

I suspect this plugin would be better off responding to the EventType::CONSTRUCTION event rather than trying to do its own detection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review In Progress
Development

Successfully merging this pull request may close these issues.

reenable infiniteSky infiniteSky not creating usable Z-levels
3 participants