Print

Please login or register.
1 Hour 1 Day 1 Week 1 Month Forever
Login with username, password and session length

[PatrikLundell]

My statement on the unsuitability to mix floating point math with integer math was intended to be general, not referring to this specific code. Floating point results end up near the mathematically correct result (most of the time: there are exceptions), but can end up both above, spot on, and below. Applying truncation or rounding up to such values can give the wrong value, where "proper" rounding might work.

The intention of the "Any" value for reanimation is "at least one of them", so it's good your results reflects that. If I'd intended to say exactly one of them I'd probably have called it "Either". However, I agree the UI would be clearer with "and/or".

I don't think there would be any reason for a player to want one or the other type of reanimation but exclude both of them, while I can see that players may want to ensure a challenge by requiring at least one of them.

[RedDwarfStepper]

My statement on the unsuitability to mix floating point math with integer math was intended to be general, not referring to this specific code. Floating point results end up near the mathematically correct result (most of the time: there are exceptions), but can end up both above, spot on, and below. Applying truncation or rounding up to such values can give the wrong value, where "proper" rounding might work.

Ah, okay, now I see. Yes, of course, you are absolutely right.

The intention of the "Any" value for reanimation is "at least one of them", so it's good your results reflects that.

Good, so I'll leave it as is.

If I'd intended to say exactly one of them I'd probably have called it "Either".

I wanted to be sure, as I experienced first hand - also with my own code in multiple projects - how software grows and changes gradually until its current internal logic no longer fits the original designations of variables, classes or display labels that were named under the impression of their initial creation - a process often imperceptible for the one perpetrating it, kind of a temporary syndrome similar to mental blindness from staring to long on to something, the last days have been full of that at work actually.
It takes a lot discipline to stay ahead of that, notice it and change names accordingly.

I don't think there would be any reason for a player to want one or the other type of reanimation but exclude both of them, while I can see that players may want to ensure a challenge by requiring at least one of them.

As I only accidentally have encountered reanimation and thralling - which ended badly quit fast - I really wouldn't dare to guess what players that are actively looking for it might want
So I'll trust your judgement.

Okay, there are 2 more things I'd need to fix before merging makes sense, but I have this idea about how to make the merging/migration to the latest version of the code both easier and safer (if for any reason all/most of my work won't make it into the plugin):
I will open 3 or 4 pull requests against your latest branch.
The first one contains a minimal change that should speed up the search without actually changing the logic.
The second one will change a little more code saving a lot of memory again without really changing any logic.
The third and if needed the fourth would both be an actually refactoring that would make the merging easier by reducing the amount of duplicated code at those places where I had to add to the code the most.

Apart from that I have to ask someone (you or lethosor?) how to handle the external dependency CRoaring/Roaring Bitmaps that I used as-is by putting the 3 files (roaring.h,~.hh,~.c) alongside the sources of embark-assistant.
Those files might make the linter scream also I'm unsure if it is fine to add external dependencies like that...
I'm willing to change the files to make them comply to the rules if necessary even if this means any future update of CRoaring brings some additional overhead.
Also some (performance) tests on Linux would be good.

Perhaps the most important question: Would you be okay with a so fundamentally changed version of the plugin?
I actually never asked that question as I never have been really sure that it could be done (there is still a chance that some "minor" detail brings it all down).
I'm willing to co-maintain with what I know, added and changed, if that makes a difference.
But there are quite a lot of DF/dfhack internals that are beyond me.
And the time I actually can spend on this is limited.
So?

[PatrikLundell]

I'm certainly aware of the process of things slowly creeping away from the original intent...

I don't mind reanimation much, but I've kept away from thralling. However, if you're willing to face thralling then reanimation isn't that much of an additional problem. People who'd search for thralling and/or reanimation are looking for a challenge.

I'm not sure how to handle the Roaring stuff. @lethosor is probably the person who can make the decision about that, but my gut feeling is that it would be better to have that as a separate addition to the DFHack code so it can be used elsewhere. Regardless, the copyright and usage conditions of the code has to be compatible with that of DFHack.

If I had been opposed to changing the plugin I would have told you so before you wasted a lot of time on the task. I have to admit I didn't think it would work, but if it does I don't see why improvements should not be made.

When it comes to merging the work, I don't know what the best process is. So far I've never merged anything but the DFHack develop/master into my branches (and then by pulling those into my local copy and then pushing it up to the branch). Usually pull requests are made against DFHack develop, and I don't really know how to handle it through my branch. The problem is that Git and I are at odds, so I can perform a very narrow set of actions with a limited risk of screwing (or be screwed) up. I don't even know how to do a diff to compare code, something that's easy in sensible version control tools such as e.g. Subversion). Advice from someone(s) experienced in this kind of Git activity would be welcome.

I'm not using Linux (or any of the other -ux variants), so I can't help with that testing.

When it comes to maintenance of the code my participation depends on the extent to which I understand what the code does.

[lethosor]

I think the multiple-PR merge strategy you mentioned would make the changes easier to digest - large merges can sometimes take some effort to go over. The current diff looks a bit large to me, but I see things split into commits well enough that it should be easier to review that way. GitHub also says there are merge conflicts, although those aren't easy to see through the web UI, so I'm not sure how extensive those are.

Merging into DFHack's develop branch is probably the way to go here, assuming it's up-to-date with PatrikLundell's latest changes.

I'm not completely sure what your changes do at the moment, mainly because I'm not particularly familiar with the plugin internals, so PatrikLundell would be a better person to make an assessment in that regard. I can do some basic testing on Linux (or more complicated testing with instructions).

Assuming you're referring to https://roaringbitmap.org/ - it appears to be Apache 2-licensed, which should be fine, although we don't currently have any dependencies using that license, and it wouldn't work particularly well with the current formatting of our license documentation page, so we might need to rework that (but I can handle that). I would also agree with PatrikLundell that it shouldn't be part of the plugin itself - it looks pretty easy to include as a dependency, and its readme even advertises that it can be included as a subdirectory, so I'm thinking the "depends" folder would be a good place for it. That's something that I could help with once you get to the point of merging it in.

[PatrikLundell]

There's nothing outstanding in my branch, as the latest PR has been merged, so that part shouldn't be an obstacle. I'll review the commits to the best of my ability.

[RedDwarfStepper]

@lethosor:
Thank you for chiming in so fast - it would have taken me another few days to address you with my questions.
And thank you very much for taking the time to get into some of the details already!

Actually my branch is not up-to-date with PatrikLundell's latest changes as I did not have the capacity to implement the adaptations and continuously merge the new features.
That explains the merge conflicts we see here.
My plan is to bring over some smaller, standalone improvements I discovered during development via separate pull requests into the develop branch by going through PatrikLundell's branch so he can review everything before it gets pushed/pulled into the main repository. After that I would fork PatrikLundell's development branch again, merge my work into it and finally add the search criteria (like tree density) that have been added since I started developing.
That seemed to be the proper way to do this to me, as his branch would stay the master/single point of truth at all times.
Also there might be a phase of consolidation before the plugin is really ready for the dfhack develop branch. To allow for easy bugfixing of the current version of the plugin a separate branch for the new stuff might be useful.
But I'm not an experienced git/github user myself so I'll happily take any advice on how to approach this.

Assuming you're referring to https://roaringbitmap.org/

You're correct - I'm using their Amalgamation/Unity Build which made the integration easy.
One reason to keep the dependency local to the plugin might be this have been the missing namespace - oh they fixed it, but the fix hasn't been propagated to the Unity Build yet, just saw that myself right now.
Well, okay - that was the one point that made me feel uneasy about putting CRoaring into the global dependency scope for all of dfhack. With that gone I'm very much in favour to treat it as any other external dependency, especially as it could be used by everyone that way, even if it is kind of a niche thing.

I'm not completely sure what your changes do at the moment, mainly because I'm not particularly familiar with the plugin internals, so PatrikLundell would be a better person to make an assessment in that regard.

If desired I could do a little write-up with the core concepts behind my additions - putting it alongside the code as a little readme might help others that look into the plugin in the future.
Also getting feedback on my ideas and design decisions after spending such a long time on it would be nice
Apart from my thousand questions about details and DF world data internals I tried to not additionally bother PatrikLundell with my thought process as it could have been for naught.

I can do some basic testing on Linux (or more complicated testing with instructions).

That would be much appreciated!
Runtime performance/speed on Linux is the one thing that could make it necessary to change part of the implementation:
I'm using std::async here. Which on Windows internally uses thread pools. But I'm not sure that is the case on Linux too by now.
And if a thread pool became necessary I really could use some hints on how to use the existing tthread dependency.
Before going for std::async I had a look at tthread and where it is being used in dfhack but it didn't make much sense then. Might be different now though.

[PatrikLundell]

Well, actually, develop is the "master" for DFHack development (with "master" basically being the latest release, and thus the stable version). Thus, basing your fork off of "develop" would be the way to go (that way the rest of the code would be up to date as well). Once your fork's pull request has been merged, you can refresh your fork with the latest from develop (which shouldn't contain any changes to the Embark Assistant code, but may refresh other things), update it with the next set of changes, and issue a new pull request.

Actually, the above is a bit of a simplification, as there are three, not two versions involved:
- On your local machine, create a fork off of develop
- Do the work
- Push it to Github
- Get comments and discussions, update the local copy and push to Github.
- Repeat until "done"
- Make a PR
- Possibly get more comments, in which case the local copy is updated and pushed to Github, which automatically amends the outstanding PR.
- Get the PR accepted and pulled into develop.
- Refresh the local copy from develop (again, typically only gets the background up to date)
- Do the work again
- etc.

If I understand it correctly, the first PR won't have any Roaring Bitmap code in it (local variables rather than .at() functions everywhere, and similar things), and so can be started immediately, while the one(s) relying on it will have to have it in place to be referenced, and so have such a PR accepted before that work can be done.

It can be noted that others have touched the Embark Assistant code without me being involved in the past, such as when the biome determination code was factored out, so my branch isn't the master.

[RedDwarfStepper]

It can be noted that others have touched the Embark Assistant code without me being involved in the past, such as when the biome determination code was factored out, so my branch isn't the master.

Ok, that is something I never knew or noticed.
You being the most vocal and present in the embark-assistant thread I assumed it all runs through you for this plugin
And all my other assumptions are based on this one.
But I was aware of another "level" concerning decisions about the whole of dfhack, like global dependencies and the like.

If I understand it correctly, the first PR won't have any Roaring Bitmap code in it (local variables rather than .at() functions everywhere, and similar things), and so can be started immediately, while the one(s) relying on it will have to have it in place to be referenced, and so have such a PR accepted before that work can be done.

You are correct in your understanding that the first few PRs won't have any references to Roaring in them at all - but at least the first 2 or 3 won't be a classic refactoring either, but smallish changes that should improve memory consumption or performance.
The refactoring comes after that and - yes again - as a preparation for the big haul.
And if you see any changes in the PRs that need clarification, more or better comments or another name for a structure than fire away.
Also I'll try to keep the changes/PRs as consistent as possible to allow for easy review and focused discussions.

Well, actually, develop is the "master" for DFHack development (with "master" basically being the latest release, and thus the stable version). Thus, basing your fork off of "develop" would be the way to go (that way the rest of the code would be up to date as well). Once your fork's pull request has been merged, you can refresh your fork with the latest from develop (which shouldn't contain any changes to the Embark Assistant code, but may refresh other things), update it with the next set of changes, and issue a new pull request.

Actually, the above is a bit of a simplification, as there are three, not two versions involved:
...

Mostly see the first paragraph about me having a faulty base assumption and not verifying properly early on which is the leading repository.
And thanks for the explanation about the workflow - on a theoretical level I'm aware that it can be done and is done like that in many public (OSS) projects.
Its the practical part of the PR process that is pretty new to me (I think I've done it twice in other OSS projects), I'm working with SVN on a daily basis and we do our "code reviews" and integration a little differently.
But I'm sure that we'll both manage just fine!

[RedDwarfStepper]

I've created the the first PR.

[RedDwarfStepper]

I started with the refactoring branch.
Will turn it into a draft/WIP PR in the next few days and notify you, so you can start reviewing at your leisure while I add some more.
There may be some small performance improvements mixed in, which will be covered by the existing changelog entry.
I'll explicitly point them out anyway, as they still deserve some more attention.

[PatrikLundell]

In retrospect, I think it would have been better if I'd implemented the improvements you've made so far when you pointed out the issues rather than wait for you to do them, as I now don't think you doing them would mean less merging work and conflict resolution than if I'd done them earlier (and thus gotten them released earlier). I guess that's a lesson for the future.

[RedDwarfStepper]

In retrospect...

Just to make sure I understood correctly - as it is late: You think the total effort is larger doing it the way we do it now? That is by "delayed" PRs which have to be reviewed and then merged which might produce merge conflicts?
If that is what you mean, than yes, it might have been less effort doing it right then and there.
But, at least for the effort on my side I can say this: I'm fine with it as it gives me the chance to learn how to use git/github properly - also it gives us the opportunity to discuss the changes more directly, closer to the code. Doing so here in the forum feels complicated sometimes.

In hindsight I would have liked to stay in sync/up to date with your development branch or at least with the develop branch of dfhack. That would have spread the merge effort evenly over the complete development time. But I was so very busy figuring out if the index approach actually would work, so I kind of couldn't be bothered with merging, also git was an even stranger beast then.

[PatrikLundell]

Not quite. I think it would have been a very slightly larger effort to add the improvements as they were discovered and then merge your "real" changes than doing it the way it's done now (because there would probably have been some merge conflicts between how you implemented it and how I did it), but with the benefit of getting the improvements out to the users a year earlier.

I'm no fan of git: it's a shotgun with a default aim at your feet, a very sensitive trigger, no safety catch, and a confusing and unhelpful tangle of obscure commands and switches.

I would probably have tried to merge changes as long as they were small and easy to do but stop when it became complex to do them (incursion introduction). At that time I'd concentrate on the proof of concept, with an eye to what effect incursions would have, get the caching to work, and then merge in incursions and adjust the caching to work with that. However, that's with some basic knowledge of git (but still probably done with a backup of my code just in case git screws me over).

[RedDwarfStepper]

Not quite. ...

Yes, I see. Had I known what I know now and found a easier and faster way to do it while getting the caching out there to the users, that would have been good.
But a lot of this was about experimenting and discovery, so even without git...

I'm no fan of git: it's a shotgun with a default aim at your feet, a very sensitive trigger, no safety catch, and a confusing and unhelpful tangle of obscure commands and switches.

It is indeed a very mighty weapon, not for the faint of heart. I consciously try to tread lightly and sometimes create branches just to test a chain of commands. In the end all question have an answer somewhere out there, but sometimes I don't know what the question is

Actually there is one question I know how to ask by now:
Comparing this line with those two - shouldn't it be

Code: [Select]

world_data->region_map[adjusted.x][adjusted.y] here and here as well?
If so I can fix is within my current branch to keep thinks simple.

And lastly should we move this specific kind of question into tickets over here?

[PatrikLundell]

Yes, you're correct. It should be "adjusted." there as like in all of the surrounding code, so yes please, take care of that one.

Personally I find it easier to discuss these issues here. I don't check issues regularly, and do it only when I'm engaged in DF structure research (or if I received a notice of an issue I should be aware of). My take is that I'd use a thread dedicated to the plugin/utility firstly if there is one, and write an issue if there isn't one or if it's unclear where the problem is. It can be noted that the DFHack thread is a general thread, not one dedicated to a specific thing, so I agree issues are better in that case to ensure things don't get lost.