Skip to content

Refactor line ingredient pasting#772

Open
vapopescu wants to merge 6 commits into
ClaudeMetz:masterfrom
vapopescu:refactor_line_paste
Open

Refactor line ingredient pasting#772
vapopescu wants to merge 6 commits into
ClaudeMetz:masterfrom
vapopescu:refactor_line_paste

Conversation

@vapopescu

Copy link
Copy Markdown
Contributor

Move pasting logic from production_handler.handle_item_click to Line.paste.

Still not ideal, because the target is not the actual line box, but it's better IMO than extending SimpleItem with a paste function.

Now also checks whether the copied temperature is valid for the target recipe, and only pastes it if it is.

@ClaudeMetz ClaudeMetz left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR. So there's a few things here I'm not sure about, I'll mention them and I'll have to sit down to think about them some more.

First, I don't like the passing a whole set of tags to :paste. Maybe it's unavoidable though. All the things that this adds though makes it not really much less of a mess than the fake object thing. It's conceptually cleaner, but this being quite a mess on Line:paste is why I thought about extending SimpleItem with a paste method. That would have its own mess though too for sure.

Second, the Recipe:set_temperature method. I agree that checking the compatibility is a good idea. I don't like making it a class method necessarily. If we do that, we should have a look at all similar situations and see if that method can be used there as well. Plus, there's a similar situation on Fuel, so that should get its own method too and be used wherever applicable. Generally, I'm not sure if a method to check compatibility would be better, and then setting it separately. The code currently just sets the .temperatures field without a method, which isn't ideal but if we add a way to do it through a method, it needs to be changed to that everywhere.

Okay that's just some thoughts, if that makes you want to change anything feel free, otherwise I'll get back to this and think through it on my own. This is the part of reviewing PRs I struggle with, if it's not as I'd expect it, it takes me a lot of energy to get over the hump of thinking through it myself. Not your fault at all, just explaining it from my perspective.

@vapopescu

Copy link
Copy Markdown
Contributor Author

We can split Recipe:set_temperature its its own PR. I think the idea is worth it. Just look how useful Recipe:get_name_with_temperature is, maybe this one could be too.

Extending SimpleItem with a paste function is also an option, but it would also require to give them a parent field. Will think some more on that too.

@ClaudeMetz

Copy link
Copy Markdown
Owner

Another option is to just make SimpleItem into proper objects. The reason I didn't is that they are just fully determined by the solver, and objects are usually something that you want to keep around, carrying some data the user has influence over. It does make it awkward to handle them alongside objects though, which is what all this is about.

So all that is why making it a pseudo-object 'feels right'. The issue with making it a full object is that it goes through a lot of unique IDs quite fast, as it uses dozens each factory solve. I don't know if that would actually be a problem, the limit is 2^53 for Lua 5.2. Probably not a limit someone would ever get to?

@vapopescu

Copy link
Copy Markdown
Contributor Author

We could reuse the indices, but it will make allocating one O(n). We could also try remapping all items to the lowest indices at game load.

But perhaps it's not necessary at all. I expect that just implementing paste and adding a reference to the line (or floor) that owns it should be enough. They just have to be a class with paste not an Object specifically.

But we should definitely move the paste logic from production_handler though

@ClaudeMetz

Copy link
Copy Markdown
Owner

Yes, agreed on all counts I think.

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants