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 conductor aliases again #615

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TurkeyMcMac
Copy link
Contributor

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.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- 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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@TurkeyMcMac
Copy link
Contributor Author

TurkeyMcMac commented May 29, 2022

Actually, I think I can fix the alias problem in conjunction with #614. vm_get_node can just do the resolving of the alias. vm_get_node_nocopy is a private function, so it's not an issue. The conversion in prepare_conductors must stay since the internal functions use vm_get_node_nocopy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants