- User Since
- Jan 5 2017, 11:29 AM (100 w, 4 d)
Sun, Nov 25
I agree. My intention is just to clarify if we can find something we all agree on so we can aim for that.
And this decision has some impact on this ticket that's why I'm bringing this up.
What's you thought in this, @elexis ?
We could avoid spreading map related files over different directories by granting maps directories and put all related files there.
(This is what I would understand what @elexis called "logically grouped". Group what is only used by one map into that folder including e.g. the map-preview. There are other possible logical groupings orc..)
Dunno what the biome discussion is about, this isn't a biome, nor does it claim to be a biome, but it extends the existing biomes, which I think only these two maps currently do.
Sat, Nov 24
Then don't pot it into a biome file.
This is just defining entities based on the biome.
...which is exactly what a biome does (alongside terrain textures and actors).
I do not think these should be put in to the biome files for the reason.
I can't follow that reasoning (see above). However, removing the camps and farms would be an alternative clean solution (though maybe not desired).
The next such map may not want to have Romans in the Alpine biome.
...and another map may not want to use that base texture for that specific biome. That's always a possibility. Also I'm absolutely fine with changing the entities.
So, I think that keeping this as part of the map would be best.
Could do. But then:
Fri, Nov 23
I find less files easier to maintain then multiple files BTW.
Thu, Nov 22
Thought I find the Obect syntax quite misleading for the content of the array is more prominent than the array itself this way.
Change documentation once more
Wed, Nov 21
(The obelisks are just meant to show the center e.g. to show where it is on fortress (the "center of mass") compared to e.g. regular polygons/circular where it's just the center of symmetry)
Tue, Nov 20
I wrote if myself but I'm pretty sure someone else did wright something similar before ;D
Now this should really be tested in action with maps but I don't know how to change the test plan or the title.
Caledonian Meadows uses this. Generate some maps, if no paths cross each other it should be fine.
Added using Vector2D.distanceTo() to simplify code.
At the moment we have:
- Terrain strings, those containing the terrain separator
- simpleTerrain objects
- randomterrain objects
- Texture strings in the Map object
- Terrain entities in the map object
did I miss something related?
Mon, Nov 19
I think this is about the second line from below ind it's placeGenericFortress()?
So I guess placement function isn't able to create any sane fortress because the radius is small compared to the long wall sizes.
If that's the case it could be fixed by:
- Change placeGenericFortress() to use smaller wall parts if the radius is smaller than the length of the "long" wall
- Change placeGenericFortress() to have a minimum radius dependent on the "long" wall's length
- Change the map to have a minimum radius for placeGenericFortress() (overlapping is not that big of a deal here since one still can determine if everything works in general IMO)
- Use larger than giant maps (not so sure about this)
Likely all of the above (or similar) should be done in the end
Sun, Nov 18
Sat, Nov 17
What is the double meaning in respect to vectors? Different objects in math have an Absolute Value but each object type's absolute is unambiguous.
It should technically be magnitude, no?
You are correct: No! ^^ The Magnitude is a ranking and thus in general less strict than a Norm. However, the norm of vector spaces can't usually be used to order them for multiple non-equal elements can have the same absolute value (see Vector spaces with additional structure).
But length is more intuitive when thinking about 2D space.
Yea, not a big deal IMO but why not make the libs as clean and clear as possible? (And what would be more clean as mathematical rigorous definitions or more widely available like Wikipedia articles sayings?)
Also you said "let point = new Vector2D(10, 10);" would make more sense to you and there is Vector.distanceTo() ... which is exactly what I would have hoped to avoid for Points are not Vectors, They don't even have the same dimensions (Point: 0, Vector: 1, the vector spaces themselves usually have n - 2 or 3 in our case). Vectors have no distances but points have, points have no differences but vectors have, especially vectors have a length and signed direction while points have positions and relations as unsigned distances.
Although, thats a pretty extreme example.
I agree, but, well, the first thing that came to mind ;)
I was the original author and am to blame. I checked the algorithm and, no, it isn't meant to shift by n so just an oversight.
Fri, Nov 16
And I wasn't when I said it's an obscured property of the reader, not the code. But since I agree with you and smiley seems to join us I don't see a problem here. And thanks for your devotion to make the code more appealing to at least us 3 (and - I expect - most of the readers).
which part of this specific patch are you referring to?
I mentioned the topic of Vector algebra briefly because the patch doesn't use Vector algebra where it presumably easily could.
No need to presume since I answered your question in the first reply on your comments on using vectors: It's ranges, not vectors.
And, yes, you can use vector algebra to calculate the volume of a cuboid that has given length of edges a, s and c by choosing an arbitrary point, say O, and an arbitrary coordinate system with an orthonormal basis (hopefully) linked to it and then determine three vectors, say v_a, v_b, v_c that have the properties that v_a.dot(v_a) == a², v_b.dot(v_b) == b², v_c.dot(v_c) == c² and v_a.dot(v_b) == 0, (v_a.dot(v_c) == 0, v_b.dot(v_c) == 0 and finally volume = v_a.dot(v_b.cross(v_c)).length() if one wants desperately to use Vectors. It also happens to be a*b*c which is shorter, faster and - I estimate - more readable to most readers. Also the prototype "length" should be called "abs" because the meaning of the resulting value depends on the case the lib's code usually doesn't know - in this case it happens to be the volume. So, no, I don't agree it's always better to use vectors, even if one could. And I originally thought that would be rather a consensus than a single point of view.
(For example the inline comment, there is Vector algebra which after some transformations would probably look much more readable. Notice the ConvexPolygonPlacer.prototype.orient function didn't came across either until I lobbied on the lobby for using the cross function, you can see it in the first version of this upload.)
And what do you expect to happen now? The author is trapped in this situation, having copied the code (well, maybe just lazy) probably not easily able to convert that to vector algebra. You demanding, but not willing to do it yourself (or check yourself if your concern is valid). And I would rather write an own patch than excepting this one due to the licensing.
But "In general" is marked in bold above and I elaborated on this topic extensively to you not because the coding convention violation of this patch is so relevant, but because I am afraid that you didn't see the reason behind the coding convention yet and might let others get away with this code style in other patches or introduce it yourself in future maps, when there might not be a very good reason to. Resist the beginnings or something.
What code convention violation? And thus what code convention's reason? I'll get code "away" after checking a long list of pros and cons when the outcome is positive. While I can write the list down (and had written it down for you more than once) it's still a mostly arbitrary decision - as is yours. The only difference I see is you think that "is" and I think that "I think". I don't allow patches in to "allow" myself to use that stile later myself ;D I don't even thing that far! If I want to have code that does something I write code that does that. If it makes sense to me to use vectors I do that. If It doesn't I don't.
Thu, Nov 15
@elexis : On Vectors: Uhm, yes, I agree on using vectors can make things easier and they should be widely used (Though much less to those "unreadable", "impossible to understand", " obscured or unrecognizable" for those are IMO obscured references to properties of the reader, not the code... This depends strongly on habituation IMO). But, yes. I think we are on the same page here. Just ... which part of this specific patch are you referring to?
Agreed, as I wrote.
(Also my alarm goes off when I see variables storing X and Z coordinates that arent vectors)
x1, x2, y1, y2 are x/y ranges so ti's absolutely valid to store them in variables (just that they are constants).
So I figure this is part of #5305 which also have some data backing up the claims so that's figured.
Still it would be very preferable to not having to add code that needs additional license comments so please try to wright this from your own understanding.
I don't think using pseudo code as a source for writing your own needs license reference.
What do you say, @elexis ?
Mon, Nov 12
Nov 9 2018
What exactly is to be optimized here? Speed?
AFAICS there's a lot more code and no tests to show the benefit.
(It also seems to be a lot more code then in the links BTW ;p)
Yea, adding a warning or at least log when biomes are missing definitions.
Otherwise I'm all for it.
Change civs for the biomes at your liking (replacing e.g. Celts in India ;p).
Sep 25 2018
Thanks @nani !
@nani terrific! Thanks!
Nice idea, @smiley .
I'd like to inform you that I requested such a feature before and was warned that the engine was not designed with this in mind.
Sep 18 2018
To make it more clear: I won't except this patch in it's current state without a pressing reason why to commit it.
To make this more clear: I'm for it and will review the patch when commits are allowed again.
Is that OK?
Aug 26 2018
Is the arbitrary "10" is gone? Can't find it any more.
This patch contains several things that are not really related to each other.
Aug 3 2018
@trigger: To encourage attacks maybe check every unit every few seconds with a random roll and maybe damage it like: if randFloat() > [units within range x of checked unit including garrisoned and structures] /100: [deal randFloat() * 10 damage to checked unit]
Yea, texture priority support is an addition that I'd like to see.
Thanks @smiley to pick this up!
@elexis True, so as you said:  it is ;)
Jul 7 2018
The comment areas.js is states:
An Area is a set of Vector2D points and a cache to lookup membership quickly.
Shouldn't it be:
- An Area is an array of Vector2D with integer components representing tile positions as position vectors and a cache to lookup membership quickly.
for Areas.getClosestPointTo uses this.points and e.g. in rmgen-common createPassage() uses arrays?
Jul 6 2018
IMO the way to solve this is:
- Identify the places that trow an arror when dealing with an empty array
- Fix them.
Getting rid of a single string with a separator character is definitely a good idea independent of the same separator in the templates (though I wonder if PPL adding stuff to the templates that brake most random maps should mind a bit more about what they are doing...).
This will make long sets of "simpleTerrain" much longer (and somewhat harder to read) though so I'd wait a bit for alternative solutions that may come up.
Dec 11 2017
My fault, didn't compile, sry.
I did apply the patch and added the raw pmp file from this ticket. Or where should I get the pmp file from?
Dec 10 2017
The map doesn't load properly:
Dec 9 2017
Everything seems to be in order.
Dec 7 2017
@elexis "Phabricator also needs 30 seconds or something to add a new inline commit which I see as a sign for moving on ;-)"
For me that is a clear sign of a ill chosen web framework ^^
(...and a reason I like trac somewhat more.
But I also like phabricators ToDo list so, OK ^^)
Dec 6 2017
I'm not convinced that validateStyle is needed in the first pace (and that _stone suffix) but OK.
Dec 4 2017
@elexis No, there isn't an actual issue right now. I mentioned those two cases that came to mind above. I don't think this will help reducing debug time consumption and is IMO a pure limitation.
I wanted to clarify that my proposal is only a simple solution, not the definite way to go.
Dec 3 2017
To change the indentation of e.g. a defense tower would require to copy the wall set, change the indentation parameter and rewrite the wall alignment function.
Without the freeze and with the wall stiles constructed at import only the indent parameter would need to be changed.
Thanks for the feedback!
Nov 12 2017
Nov 11 2017
The wall lengths are really good! Thanks much!
Nov 5 2017
Thanks much for picking this one up @s0600204 !
Oct 7 2017
Oct 4 2017
Sep 8 2017
@elexis Ultimately, yes. But ATM there are 2 kinds of templates in the directory templates:
- Definitions to generate entities from (in the subdirectories). Entity templates so to say
- Perdefinitions of of a set of those (with the prefix template_), Entity template templates so to say.
Both are templates but in a different way.
@Stan pointed out that since the trigger points are moved to the directory triggers the trigger prefix should be removed.
I agree since this was the default approach previously, too.
So this should be part of this commit to stay conform.
I somehow agree with elexis meaningful folder naming.
Still I consider this is a welcome improvement ;)
Sep 6 2017
@linear gain: Then I'd always chose the shortest possible trade routes since it's the same amount of resources per time and trader no matter the distance ... and nearly no risk involved.
@square is to much: What about an exponent about 1.5?
Aug 19 2017
Thanks for the fast and pleasant feedback, @fatherbushido.
I have not looked into the other patch so please commit the two ;)
Aug 18 2017
This patch has been reviewed and accepted and I also agree with it.
Aug 17 2017
Jul 29 2017
Jul 17 2017
Jun 20 2017
Rebased after changeset 19807 (D659 committed)
To reproduce the original bug:
Generate: Map Size: Normal, Number of Players: 4, Seed: 2537 (High ground trees in bases of player 1 and 4)
Jun 19 2017
I'd add a comment to the documentation of getTileCenteredHeightmap() that TILE_CENTERED_HEIGHMAP must be false (see comment at the top of heightmap.js) for this is the point where things would break.
Otherwise perfect, thanks much, @elexis !
Wait with this after D659 !
Jun 8 2017
Jun 6 2017
Jun 1 2017
Fixed most of elexis comments
Increases defenders of the mercenary camps
Larger map sizes also have smaller lakes (maybe that initial heightmap would work well for all map sizes)