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

The code to provide a city block tracker #52

Merged
merged 55 commits into from
Jul 31, 2023
Merged

Conversation

drdozer
Copy link
Contributor

@drdozer drdozer commented Jun 29, 2023

See #45

drdozer and others added 26 commits June 26, 2023 17:38
@drdozer
Copy link
Contributor Author

drdozer commented Jun 29, 2023

I'm getting this error:

Error while running event TLBE::on_gui_selection_state_changed (ID 60)
__TLBE__/scripts/gui.lua:1560: attempt to index field 'tlbe-tracker-cityblock-size-x' (a nil value)
stack traceback:
	__TLBE__/scripts/gui.lua:1560: in function 'updateTrackerConfig'
	__TLBE__/scripts/gui.lua:1507: in function 'createTrackerConfigAndInfo'
	__TLBE__/scripts/gui.lua:459: in function <__TLBE__/scripts/gui.lua:408>

I'm not quite sure why it is happening. I copied and adapted the code for the area tracker, so I am not sure why it can find the area tracker fields but not the city block fields.

@veger
Copy link
Owner

veger commented Jun 29, 2023

You put the city block GUI inside its own 'flow' (here) and on line 1560 you are reading from the 'main' tracker flow (trackerInfo). So your field is not found (nil) and you call text on it. Causing the error.

Instead you first need to fetch that 'flow' and then index your fields, like it is done here

@veger
Copy link
Owner

veger commented Jun 29, 2023

BTW You master branch is outdated, that is why you see all your (old) commits... You need to (re)sync it with the main/this repo.
With forks (but in general as well) you should create a 'feature branch' and work in there. So the main/master branch stays clean and is easy to sync.

@drdozer
Copy link
Contributor Author

drdozer commented Jul 1, 2023

  • Changing the decimal of the scale is very awkward: delete the 5 and a 0 pops up, so I cannot fill in 1.25 for example.

I've fixed the update function to round to two sf, just like the display function does. This should be fixed by that, but GUIs are eldrich horrors.

@drdozer
Copy link
Contributor Author

drdozer commented Jul 1, 2023

  • Changing the decimal of the scale is very awkward

Yes -- it is a weird interaction of the logic for setting the value into the field and setting the value into the data. I will have a cup of tea and maybe ask in discord, as I'm a little stumped as to how to fix it.

@drdozer
Copy link
Contributor Author

drdozer commented Jul 1, 2023

GUIs are eldrich horrors

I'm a bit stuck with this issue of the scale decimal value. I think I need to validate/set the value in some event that is not onTextChanged, as it is "fixing" the number on keypress, which means it gets reformatted on each keypress.

…s <enter> to commit a new value. Tooltip reflects this.
@drdozer
Copy link
Contributor Author

drdozer commented Jul 1, 2023

Possible solution pushed.

@veger
Copy link
Owner

veger commented Jul 1, 2023

GUIs are eldrich horrors

agree yes... unfortunately they are a necessary evil sometimes.... 👿

I'll fetch your newest changes, and give it another try

Copy link
Owner

@veger veger left a comment

Choose a reason for hiding this comment

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

Pressing enter to set the scale fixes the weirdness, but it is not the same as the rest of the GUI (or even game) functions... But (for now) I do not see another more suitable (GUI) event to solve this...

Edit If the element is not updated (only the setting), its behavior won't be weird. Only the GUI might not reflecting the actual state (at all times)... 🤔

It is not breaking anything, but I think allowing block size = 0 is not good.
A city block of 1 is also kind of weird/small, but the calculated center is looking better (like it works)

Edit Setting size to 0,0 and clicking 'select block'/player button messed up centerPos. So not allowing 0 here is really needed.

Also, the game hung on me once, had to force quit (I think while I removed/cleared offset-x, but I cannot reproduce anymore)

scripts/tracker.lua Outdated Show resolved Hide resolved
control.lua Outdated Show resolved Hide resolved
@drdozer
Copy link
Contributor Author

drdozer commented Jul 2, 2023

I think allowing block size = 0 is not good.

Agreed. Now, let's see if I can address this without more GUI eldrich horrors being invoked. I've pushed what I hope is a fix.

@drdozer
Copy link
Contributor Author

drdozer commented Jul 2, 2023

Also, the game hung on me once, had to force quit

It has hung on me a couple of times also. I have not been able to work out where or why it is hanging. My first thought was some interaction with the vs code plugin. I haven't yet had a hangup when running factorio free-range.

@drdozer
Copy link
Contributor Author

drdozer commented Jul 4, 2023

Currently without Internet due to a broken 4g mast in our area...

@veger
Copy link
Owner

veger commented Jul 20, 2023

I would like to include this new tracker in the coming release, which I plan do to soon-ish so others can enjoy all the new additions to the mod.

Can you indicate whether you can spend some time on this PR, or shall I release without the new city block tracker?

@drdozer
Copy link
Contributor Author

drdozer commented Jul 31, 2023

Sorry - life happened. What's outstanding for this to be released?

@veger
Copy link
Owner

veger commented Jul 31, 2023

Sorry - life happened

No worries. Same here, so I didn't do a release yet

I think the following issues are still open:

I think they are mostly small clean up changes

@drdozer
Copy link
Contributor Author

drdozer commented Jul 31, 2023

I think they are mostly small clean up changes

I think they are all now resolved.

@veger veger merged commit e3c4c22 into veger:master Jul 31, 2023
1 check passed
@veger
Copy link
Owner

veger commented Jul 31, 2023

nice thanks a lot for this feature!

I'll try and find some this later to make the release

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.

2 participants