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

Bugfix/maps over 4km #24

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ArchieHill
Copy link
Contributor

@ArchieHill ArchieHill commented Jun 11, 2023

X Test.csv
Y Test.csv

I've noticed that maps over about 4km have their first line of elevations overwritten as the map can't expand further in that direction.

My solution has just been to move the map in the opposite direction to how its constructed the amount of squares the map exceeds 520 (4160m) in which the editor can then start as normal but with the extra space to expand properly.

Using the two tests attached I've proven that my fix works but why the y axis needs an extra 7 spaces to properly function I'm not sure. Maybe something to do with the margin calculations? Its tough to figure out when nothings commented, so any guidance as to why would be appreciated.

Link to a discussion of the same problem encountered on the CM forum:
https://community.battlefront.com/topic/142417-creating-maps-with-cmautoeditor/page/5/

@DerButschi
Copy link
Owner

Thanks for looking into it, much appreciated! (Really, despite what I write below.)

BUT: You have much to learn, young Padawan!

  • First of all: Humility. "You noticed", "your solution", "bugfix", a month after it was discussed in the thread, where I described the problem and hinted at the solution? Where I describe that it isn't even a bug but due to the limitations of the editor? And that I never even meant to go beyond 4160m? That's... imaginative...
  • Second: Testing. If you talk large you had better deliver! Your tests with which you've "proven" your "fix" works are incomplete. Your solution fails when having more than one "page" in each direction.
  • Third: Reading. Re-read what I wrote about the rules of contributing. I think I made it sufficiently clear, that I do this for fun and don't like to be told how I should write my code. If I'm in the mood to comment my code, I will, otherwise, I won't. Period. Final statement. Also: Before you complain about commenting, read PEP-8: "Start each line with a # followed by a single space." ;-)

@DerButschi
Copy link
Owner

DerButschi commented Jun 13, 2023

If you look at this file
scroll_test_large.csv
you can see where your solution fails. (It's not too far off, though).

The margins, btw., are absolutely necessary. Otherwise building placement won't work properly.

@ArchieHill
Copy link
Contributor Author

ArchieHill commented Jun 16, 2023

  • First of all: Humility. "You noticed", "your solution", "bugfix", a month after it was discussed in the thread, where I described the problem and hinted at the solution? Where I describe that it isn't even a bug but due to the limitations of the editor? And that I never even meant to go beyond 4160m? That's... imaginative...

I genially was not aware that this had been discussed in the CM forums, I don't frequent it preferring instead to hang out in the CM discord. I'm not surprised though that someone else also discovered this. Is it so unbelievable that I came up with the same idea for the solution when its the most obvious one? You're right that its a limitation rather then a bug fix, its really not that deep, I just created the branch when I thought it was a bug and then discovered it wasn't. I also didn't know you never intended for the tool to go past 4160m, to me at least the idea of the project was to assist people with making large maps with the tool having the most effect on the largest maps which go beyond this limitation.

I've edited the original pull request comment to include a link to the discussion made on the forum.

  • Second: Testing. If you talk large you had better deliver! Your tests with which you've "proven" your "fix" works are incomplete. Your solution fails when having more than one "page" in each direction.

Seems I've portrayed far more confidence then I actually felt. I was fearing this might be the case and was just hoping it wasn't. I think this demonstrates perfectly what I'm missing in knowledge on how the system works. I don't understand how moving the map at the start is somehow affecting the calculations of everything else, as far as I can tell it has no knowledge of where it is on the map and I'm not editing any of the original variables.

  • Third: Reading. Re-read what I wrote about the rules of contributing. I think I made it sufficiently clear, that I do this for fun and don't like to be told how I should write my code. If I'm in the mood to comment my code, I will, otherwise, I won't. Period. Final statement. Also: Before you complain about commenting, read PEP-8: "Start each line with a # followed by a single space." ;-)

I'm well aware you don't want to comment the code you've told me before, my intention wasn't to complain and demand you add them in. I was trying to ask for a little help and insight into why I might need a magic number on top of the margins as I'm struggling to understand why myself. I just like to give a reason for why I think I am stuck (Something my job has hammered into my brain), even if everything was commented I might still be stuck and then I would give a different reason.

I'll keep chipping away at this with the csv you've provided and hopefully have something that works soon.

@ArchieHill
Copy link
Contributor Author

ArchieHill commented Jun 16, 2023

scroll_test_large_rotated.csv

I believe I've now fixed it, the problem was the calculations were being done purely on squares extra needed, this would cause it to fail if say 2.5 extra pages of squares were added as the editor would try and increase all the way to 3 pages of squares. Redoing it to be based on the number of pages has passed both the tests I created, the test you created and the test I've attached which is your test but I switched the x and y around to rotate the rectangle. So hopefully this is acceptable and no bugs are present.

The reason for so many changes that don't seem to do anything is because my formatter did something weird and I couldn't get it to undo the changes.

@DerButschi
Copy link
Owner

Great, thanks! Currently I'm a bit busy with RL but looks good at first glance (apart from the editor mess... ;-)). I'll take a closer look as soon as I find the time.

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

Successfully merging this pull request may close these issues.

None yet

2 participants