-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
$variable
assignment no longer seems to work.
#5075
Comments
I don't have a strong opinion, and I'm happy to change to a different way of doing it if it's proportionally similar in ease-of-use. However, I imagine this regression will break a lot of existing code too. So compatibility might be an important consideration. |
This case is not a bug, but is actually a straightforward textbook case where you ended up fixing your code to be correct. You were using |
I understand dynamic scope variables, so I guess my question is, are default arguments evaluated according to lexical scope, or at the time of the method call? The latter would seem to be the behaviour I was depending on previously. With my recent change, I suppose I can no longer change this value globally from a different file. Is that correct? In other words, I think I lost some functionality. |
Default arguments are evaluated at the time of the call. What was happening in the previous bug, was that the top scope of the use<> file was being dynamically re-evaluated at each call of each function and each module, as if the execution jumps to the top of the use<>d file, runs through that, and then down to whatever function or module you were calling, each time. Because dynamic scope is being used properly now, actually you gained the ability to pass in $radial_error, whereas your previous code in the old version of the program would have ignored any value you tried to define globally outside, overwriting it with your lexical scope definition of a dynamic scope variable, perhaps contrary to your intuition. (Maybe you only intended for this to be possible before, but didn't try it, but it appears to me that structure of code you had before fully blocks external control of the values under the old bug.) To get dynamic behavior with a default set in a single location, try using is_undef like this:
Or you can write similar is_undef routines in the parameter lists, inside of let in functions, or inside of modules if you want. |
That makes sense. The only thing I'd suggest here, is that if we can detect the previous usage, we should issue a warning and ideally link to some kind of documentation which explains how to fix it. |
Detection is likely impossible without false positive triggers, because people often put other code (e.g., test code) in use<>d files for usage when the file is loaded directly, but that is intended to be ignored when it is use<>d, as this is the specification of how use<> is supposed to work. From the manual: |
Not sure it can be called a bug when the official OpenSCAD examples use it. For example, the second one uses $ variables at file scope. |
That is still the case as you can define a normal variable at file scope and use it in a function or module. And $variables are still evaluated at file scope and can be used by file scope expressions. The change is they can't be seen directly in functions and modules, but only when the file is used. They work if it is the top level file. And they can be used indirectly in functions and modules if you copy them to a normal variable at file scope. |
@rcolyer wrote:
But... it still does that now. So we have the worst of all worlds: the overhead (and arguably incorrectness) of evaluating those assignments, and incompatibility with previous behavior. |
Yes proper fix would be to put the files cope search back but only after the stack search and then fix the performance problem by tagging expressions that don't need re-evaluation because they are true constants, i.e. don't use $variables or depend on other expressions that use $variables or call functions that use $variables. Probably 99.9% of my file scope expressions are constant but the odd one isn't. |
Analyzing expressions to determine whether they are constant is not at all trivial. Constant-ness would need to percolate up through arbitrary levels of expressions and function calls. |
Not trivial but not overly complex either. Even if regards all function calls as none constant it would save millions of re-evaluations in my code and probably speed it up from 4 minutes to a few seconds. |
In order to get my model to work on the latest version of OpenSCAD, I had to make the following change:
ioquatix/matx-case@50a230f
While I'm not a big fan of mutable globals, this was quite a useful feature for having standard modules that could be globally customized.
In other words, occasionally, I'd do something like this:
I'm not sure if there is a better alternative. I also understand that this isn't exactly "good code" as there is a chance for having conflicting assignments etc.
The text was updated successfully, but these errors were encountered: