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

Orders tab with new Mine action #1640

Merged
merged 8 commits into from
Dec 12, 2016

Conversation

dusho
Copy link
Contributor

@dusho dusho commented Nov 20, 2016

There is a new tab 'Orders' where I moved Deconstruct and added new Mine orders. So it is possible to use drag to select area for mining.
image
New tab looks like this:
image
There is new folder in Models called OrderActions that contains classes (currently Build, Deconstruct and Mine) with job creation and deserialization logic. I have removed FurnitureConstructionJob and FurnitureDeconstructionJob prototypes (did so also for utilities).
Also I experimented with parenting for furniture (have it functional somewhere in git stash), but there was really no benefit as every furniture is unique enough and doesn't share stuff with other furniture.
Please give it a try if you have time and report any issues.

@koosemose
Copy link
Collaborator

On the future Expansion front, I'm not convinced TypeTags is the right thing to attach actions to. I'd prefer a separate Actions System, not dissimilar to EventActions, dispatched to a furniture (or utility or whatever else) along the lines of Action(string actionName) the main difference being that we would want some to have default behavior, and some to be on by default (such as with deconstruction of course).

@dusho
Copy link
Contributor Author

dusho commented Nov 22, 2016

Yes. Was thinking about something similar. Though I have yet to come up with idea to have default behaviors and still have it somehow in .xml for modders to see and override.
One solution I saw used when modding Torchlight back in days is that there is some base definition that things will inherit from which has the default and common definitions.
So e.g. there will be furniture definition GenericFurniture (generic_furniture) with common actions and you can put here even that #1609 in some way to not have it in all furniture definitions. Then every furniture that uses same things will have in definition:

<Furniture type="door" inheritFrom="generic_furniture">
        <Name>Door</Name>
        ...
</Furniture>

This could even decrease the size of furniture definitions if there will be more base definitions. Not sure whether it makes sense to restrict inheritance level to 1, to prevent complex mess, but again, could clean up the definitions.

@koosemose
Copy link
Collaborator

That doesn't look a half bad way to do it, though I would have it by default inherit from generic_furniture, if no other inherit is set, no reason to have identical code that has to be on every furniture, since it would be the more common. And I don't see any reason to artificially limit inheritance depth, (unless there's a point at which it becomes a performance concern), don't necessarily need excess effort to implement it if it doesn't just happen as a result of basic implementation (or isn't too much excess effort). If a modder wants to do something more complex that such things makes easier, more power to them, it should however be discouraged in core, unless doing so is required or prevents things from being excessively complex.

@dusho dusho changed the title [WIP][Feedback] Orders tab with new Mine action [Feedback] Orders tab with new Mine action Dec 4, 2016
@dusho
Copy link
Contributor Author

dusho commented Dec 4, 2016

updated
When testing I saw issues unrelated to this:

  • when building walls, characters gets stuck inside it (pathing errors)
  • occasional problem that characters get stuck in hauling job inside stockpile (again pathing issue, I guess) when job is removed, starts working again
  • sometimes on 'Play' game just breaks with weird errors, just re-starting game again fixes things (probably something to do with that wretched localization code)

@bjubes
Copy link
Collaborator

bjubes commented Dec 8, 2016

When using Deconstruct Furniture and dragging over a large area that includes some furniture, nothing is marked for deconstruction and I get this error

InvalidOperationException: Operation is not valid due to the current state of the object
System.Linq.Enumerable.Last[Job] (IEnumerable`1 source)
BuildModeController.DoBuild (.Tile tile) (at Assets/Scripts/Controllers/InputOutput/BuildModeController.cs:313)
MouseController.BuildOnDraggedTiles (.DragParameters dragParams) (at Assets/Scripts/Controllers/InputOutput/MouseController.cs:449)
MouseController.UpdateDragging () (at Assets/Scripts/Controllers/InputOutput/MouseController.cs:325)
MouseController.Update () (at Assets/Scripts/Controllers/InputOutput/MouseController.cs:129)
MouseController.<MouseController>m__23 (Single time) (at Assets/Scripts/Controllers/InputOutput/MouseController.cs:61)
TimeManager.InvokeEvent (System.Action`1 eventAction, Single time) (at Assets/Scripts/Models/Events/TimeManager.cs:198)
TimeManager.Update (Single time) (at Assets/Scripts/Models/Events/TimeManager.cs:125)
GameController.Update () (at Assets/Scripts/Controllers/GameController.cs:77)

also bonus points if mining in dev mode caused asteroids to be insta-mined, like how deconstruction is handled.

@koosemose
Copy link
Collaborator

Conflicted

@koosemose koosemose changed the title [Feedback] Orders tab with new Mine action [Conflicts][Feedback] Orders tab with new Mine action Dec 9, 2016
@dusho
Copy link
Contributor Author

dusho commented Dec 11, 2016

Unconflicted. Works with developer mode now.
@bjubes regarding deconstruct - I haven't touched existing code.. but yes, it's wonky

@dusho dusho changed the title [Conflicts][Feedback] Orders tab with new Mine action Orders tab with new Mine action Dec 11, 2016
@koosemose
Copy link
Collaborator

To confirm, the deconstruct bug exists in master.

@koosemose
Copy link
Collaborator

One small issue: The Inventory Spawning display appears over top of the orders tab, I believe the Construction menu solves this by hiding the inventory spawning window when the construction menu is shown, and reshowing it when the construction menu is hidden. Other than that, looks good.

@bjubes
Copy link
Collaborator

bjubes commented Dec 12, 2016

Because both of the bugs noted exist on master, merging

@bjubes bjubes merged commit 88af2d0 into TeamPorcupine:master Dec 12, 2016
@koosemose
Copy link
Collaborator

Actually the issue with the inventory spawn window overlaying the orders menu doesn't exist in master... but ok, I guess...

@dusho dusho deleted the feature/orders-tab-mine branch January 13, 2017 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants