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
Conversation
\o/ |
…cope in a single place.
Identify variable-scopes using a dedicated class instead of feature numbers. The later fails for towns, which have no feature number.
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 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most messages in this file have a trailing colon.
Some of those which do not end with ".", end with ":" followed by some source quotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems correct, and it's a lot more cleaner than the old way.
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 )