-
Notifications
You must be signed in to change notification settings - Fork 121
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 conductor aliases again #615
base: master
Are you sure you want to change the base?
Conversation
@@ -403,6 +403,7 @@ function mesecon.vm_get_node(pos) | |||
end | |||
|
|||
-- Sets a node’s name during a VoxelManipulator-based transaction. | |||
-- Aliases may not work correctly. |
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.
-- Aliases may not work correctly. | |
-- Aliases may not work correctly if mesecon.get_conductor(name).states was touched by a mod. |
That's how I understood the problem, right? If you want to make sure they're not modified, consider metatables. At least the reason why it may not work correct should be stated here.
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.
The issue can occur if a mod swaps in a node whose name is actually an alias.
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.
Then please be so nice to mention in which cases this function is expected to work, and when not.
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.
I will make some changes after #614 is merged or closed. If it is merged, I can make some further changes to the code then remove the notice. This PR is basically blocked for now.
Actually, I think I can fix the alias problem in conjunction with #614. |
I think they were broken by #600, since the name of a node from the cache may now be an alias. So as not to risk slowing down the oft-called
mesecon.vm_swap_node
, I decided to fix this by resolving any aliases beforehand. This has the added benefit of fixing conductors with more than two states which have aliases in their state lists; before they would crash. The downside is that mesecons specifications may be modified, which is not documented. On the other hand, it isn't documented that they won't be modified, either. Sharing state lists between multiple conductors still works.The tests from #605 pass.