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

Refill on the field, for now only stationary waiting until refilled. #3505

Merged
merged 14 commits into from
Oct 11, 2024

Conversation

schwiti6190
Copy link
Contributor

@schwiti6190 schwiti6190 commented Oct 5, 2024

  • Works for sowing machines, sprayers and tree planters.

TODO:

  • Translation texts are not really understandable
  • All the test scenarios ...

schwiti6190 and others added 2 commits October 6, 2024 00:36
- Works for sowing machines, sprayers and tree planters.

TODO:
- Translation texts are not really understandable
- All the test scenarios ...
@schwiti6190 schwiti6190 requested a review from Tensuko October 5, 2024 22:37
@schwiti6190 schwiti6190 changed the title Refill on the field Refill on the field, for now only stationary waiting until refilled. Oct 5, 2024
<Text language="en"><![CDATA[Refill tool]]></Text>
</Translation>
<Translation name="CP_vehicle_setting_refillOnTheField_tooltip">
<Text language="de"><![CDATA[Gerät wartet auf dem Feld zum auffüllen.]]></Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

zum Auffüllen.

Außerdem würde ich den Satz umstellen:

Gerät wartet zum Auffüllen auf dem Feld.

oder

Gerät wartet auf dem Feld, um aufgefüllt zu werden.

@Tensuko Tensuko marked this pull request as ready for review October 11, 2024 08:46
@Tensuko Tensuko requested a review from pvaiko October 11, 2024 08:47
return self.held:get()
local isHeld = self.held:get()
if not isHeld then
self.fuelSaveActiveWhileHeld = false
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't very clean, one would not expect a query function like this to have side effects.

end

function AIDriveStrategyFieldWorkCourse:updateFilling()
--- Trys to load if possible
Copy link
Contributor

Choose a reason for hiding this comment

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

tries

--- Waiting for refilling on the field
-----------------------------------------------------------------------------------------------------------------------

function AIDriveStrategyFieldWorkCourse:prepareFilling()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot of code in these *Filling() functions, not really related to the strategy, maybe move some to ImplementUtil or similar, or its own helper, keeping only the actions/state changes here?

@@ -499,11 +490,12 @@ end
--- Hold the vehicle (set speed to 0) temporary. This is meant to be used for other vehicles to coordinate movements,
--- for instance tell a vehicle it should not move as the other vehicle is driving around it.
---@param milliseconds number milliseconds to hold
function AIDriveStrategyCourse:hold(milliseconds)
function AIDriveStrategyCourse:hold(milliseconds, fuelSaveAllowed)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably missed them, but there aren't a lot of comments on how this refill should work, are we using hold()? If so, can we update the comments to make it clear?

@@ -25,13 +25,15 @@ function CpAIJobFieldWork:setupTasks(isServer)
self.attachHeaderTask = CpAITaskAttachHeader(isServer, self)
self.driveToFieldWorkStartTask = CpAITaskDriveTo(isServer, self)
self.fieldWorkTask = CpAITaskFieldWork(isServer, self)
-- self.refuelTask = CpAITaskRefuel(isServer, self)
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code

end

function CpAIJobFieldWork:onPreStart()
CpAIJob.onPreStart(self)
self:removeTask(self.attachHeaderTask)
self:removeTask(self.driveToFieldWorkStartTask)
self:removeTask(self.fieldWorkTask)
-- self:removeTask(self.refuelTask)
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code

@@ -40,15 +42,38 @@ function CpAIJobFieldWork:onPreStart()
end
self:addTask(self.driveToFieldWorkStartTask)
self:addTask(self.fieldWorkTask)
---TODO: only if refuelling is possible ..
-- self:addTask(self.refuelTask)
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code

end

function CpAIJobFieldWork:setupJobParameters()
CpAIJob.setupJobParameters(self)
self:setupCpJobParameters(CpFieldWorkJobParameters(self))
end

function CpAIJobFieldWork:isFinishingAllowed(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

just a little comment on what this means? Or is this a Giants function? Also some explanation on the concept here, especially since this is again a getter function with side effects.

CpAITask.reset(self)
end

function CpAITaskFieldWork:setStartPosition(startPosition)
self.startPosition = startPosition
end

function CpAITaskFieldWork:setWaitingForRefuelActive()
Copy link
Contributor

Choose a reason for hiding this comment

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

Refuel can be confusing, it sounds more like filling the tractor with diesel. Can we use refill everywhere?

@@ -341,7 +339,12 @@ function CpAIWorker:stopCurrentAIJob(superFunc, message, ...)
end
end
end
local job = self:getJob()
if not job:isFinishingAllowed(message) then
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, so the idea is intercept the stopCurrentAIJob() from the implements when empty? Can't we handle that within the strategy?

@schwiti6190 schwiti6190 requested a review from pvaiko October 11, 2024 17:00
@Tensuko Tensuko merged commit 92fbb8e into main Oct 11, 2024
4 checks passed
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.

4 participants