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

Fix: Access to persistent storage of towns #173

Merged
merged 4 commits into from May 23, 2021
Merged

Conversation

@frosch123
Copy link
Member

@frosch123 frosch123 commented Dec 3, 2020

This PR adds a distinction between 'features' and 'scopes' (defined by feature+self/parent), instead of identifying both with interchangeable integers.
This fixes access to persistent storage to towns. (see #26, #28, https://www.tt-forums.net/viewtopic.php?p=1220012#p1220012 )

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Dec 3, 2020

\o/

@frosch123 frosch123 force-pushed the frosch123:features branch from 043ed03 to 225bc7a Dec 4, 2020
@frosch123 frosch123 force-pushed the frosch123:features branch from 225bc7a to 8ecb615 Feb 22, 2021
@frosch123 frosch123 marked this pull request as ready for review Feb 22, 2021
frosch123 added 3 commits Feb 22, 2021
Identify variable-scopes using a dedicated class instead of feature numbers.
The later fails for towns, which have no feature number.
@frosch123 frosch123 force-pushed the frosch123:features branch from 8ecb615 to 7c05d8b Feb 22, 2021
@Andrew350
Copy link

@Andrew350 Andrew350 commented Feb 26, 2021

I'm not sure what it's worth, but I just tested this PR with my NewGRF, and I'm now able to successfully store a value via an industry and then read that value from a house to conditionally allow/disallow construction. So it seems to work for me 🙂

Copy link
Member

@LordAro LordAro left a comment

Code looks fine, can't speak for its correctness

self.parent_scope = parent_scope

def get_scope(self, var_range):
assert var_range in (0x89, 0x8A)

This comment has been minimized.

@LordAro

LordAro Mar 22, 2021
Member

asserts get removed in optimised builds (like Windows exe), and I think most others have been replaced with an explicit raise AssertionError. Not necessarily an issue, but something to consider?

This comment has been minimized.

@frosch123

frosch123 Mar 30, 2021
Author Member

We removed all "assert False". If they were removed in release builds, they would open up new code paths.
A tool warned about that.
Regular asserts are fine.

var_feature = action2var.get_feature(self) # Feature of the accessed variables
var_scope = action2var.get_scope(feature, self.var_range)
if var_scope is None:
raise generic.ScriptError("Requested scope not available for this feature.", self.pos)

This comment has been minimized.

@LordAro

LordAro Mar 22, 2021
Member

separate issue to deal with, but most ScriptErrors do not have a trailing '.':

$ grep -R "raise generic.ScriptError" nml | grep '[.]"[^"]*$' | wc -l
131
$ grep -R "raise generic.ScriptError" nml | grep '[^.]"[^"]*$' | wc -l
284

This comment has been minimized.

@frosch123

frosch123 Mar 30, 2021
Author Member

Most messages in this file have a trailing colon.
Some of those which do not end with ".", end with ":" followed by some source quotation.

@glx22
glx22 approved these changes May 18, 2021
Copy link
Contributor

@glx22 glx22 left a comment

Seems correct, and it's a lot more cleaner than the old way.

@frosch123 frosch123 merged commit 4444c13 into OpenTTD:master May 23, 2021
21 checks passed
21 checks passed
@github-actions
Commit checker
Details
@github-actions
Python 3.5 on ubuntu-latest
Details
@github-actions
Security and Quality Security and Quality
Details
@github-actions
Python 3.6 on ubuntu-latest
Details
@github-actions
Python 3.7 on ubuntu-latest
Details
@github-actions
Python 3.8 on ubuntu-latest
Details
@github-actions
Python pypy3 on ubuntu-latest
Details
@github-actions
Python 3.5 on macOS-latest
Details
@github-actions
Python 3.6 on macOS-latest
Details
@github-actions
Python 3.7 on macOS-latest
Details
@github-actions
Python 3.8 on macOS-latest
Details
@github-actions
Python 3.5 on windows-2016
Details
@github-actions
Python 3.6 on windows-2016
Details
@github-actions
Python 3.7 on windows-2016
Details
@github-actions
Python 3.8 on windows-2016
Details
@github-actions
Python 3.x on ubuntu-latest
Details
@github-actions
Python 3.x on macOS-latest
Details
@github-actions
Python 3.x on windows-2016
Details
@github-actions
Flake8
Details
@github-actions
Black
Details
@github-code-scanning
CodeQL 2 new notes, 2 fixes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants