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.

Sign up for GitHub

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

Jump to bottom

Revamp Time Cost Feature #4840

Closed
wants to merge 54 commits into from
Closed

Revamp Time Cost Feature #4840

wants to merge 54 commits into from

Conversation

cochcoder
Copy link
Contributor

@cochcoder cochcoder commented Apr 1, 2024 •
edited
Loading

For whatever reason GitHub decided to close the original PR which was #4392, that's why I reopened it here

EDIT: Forgot to add this: closes #3502

cochcoder and others added 30 commits March 8, 2024 19:19
@cochcoder
Add electric_consumption & electric_cost
c3800bf
@cochcoder
Change time_cost to electric_consumption & add electric_cost
6c28d4f
@cochcoder
Add electric_consumption & electric_cost
894913f
@cochcoder
Add electric_consumption & electric_cost
2e5242c
@cochcoder
Update total cost caculator
6d7cc68
@cochcoder
Fix total_cost caculator
887819b
@cochcoder
Attempt at fixing incorrect cost calculation
52478fe
@cochcoder
Slight tidying up
720b9a0
@cochcoder
Merge branch 'SoftFever:main' into main
e227687
@cochcoder
Add electric cost section to legend
bb83dec
@cochcoder
Misc.
c25372f 
Rename total_cost to total_filament_cost
Add electric_cost to print_statistics
Rename Cost in legend to Filament cost as well as changing print statistic used
@cochcoder
Hopefully this fixes failed builds
6665bd8
@SoftFever
Merge branch 'main' into main
39953b0
@cochcoder
Fix failed builds attempt 2
d5be7a7 
Re add electric_cost to print_statistics
Add total_filament_cost & electric_cost to PrintStatistics in Print.hpp
@cochcoder
Merge branch 'SoftFever:main' into main
cc3a1a2
@cochcoder
Replace total_cost with total_filament_cost
69308cc
@cochcoder
Forgot to replace this...
626f1e1
@cochcoder
Edit legend
6412a38
@cochcoder
Merge branch 'SoftFever:main' into main
a3a83b1
@cochcoder
Forgot this one
00e6c12 
Note to self, most likely will need to replace c_str with something
@cochcoder
Add Total Cost to legend
e4770fe 
Renamed electric_cost, in most areas, to kWh_cost to fix a bug
Created a formula for calculating the total cost
@cochcoder
Add Filament cost & Electric cost to All Plates Stats
292672d
@cochcoder
Attempt at fixing failed builds
80da975
@cochcoder
Remove redundant declaration of buf
594508f
@cochcoder
Add if statment
dcc93d3 
If the user enters both the electric consumption and kWh price then separate costs will be shown.
@cochcoder
Oops
e5cf92c
@cochcoder
Syntax errors
3b02513
@cochcoder
Some fine tuning
0a7b057
@cochcoder
Unneeded +
ae779f0
@cochcoder
Merge branch 'SoftFever:main' into main
a27527c
cochcoder added 14 commits March 12, 2024 11:25
@cochcoder cochcoder mentioned this pull request Apr 1, 2024
Closed
8 tasks
SoftFever
SoftFever reviewed Apr 14, 2024
View reviewed changes
src/libslic3r/Fill/FillRectilinear.cpp
@@ -2564,10 +2564,10 @@ static std::vector<MonotonicRegionLink> chain_monotonic_regions(
}

// Reinforce the path pheromones with the best path.
float total_cost = best_path_length + float(EPSILON);
float total_filament_cost = best_path_length + float(EPSILON);
Copy link
Owner

Choose a reason for hiding this comment

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

can you revert the changes here?

SoftFever
SoftFever reviewed Apr 14, 2024
View reviewed changes
src/libslic3r/Fill/FillRectilinear.cpp
for (size_t i = 0; i + 1 < path.size(); ++ i) {
MonotonicRegionLink &link = path[i];
link.next->pheromone = (1.f - pheromone_evaporation) * link.next->pheromone + pheromone_evaporation / total_cost;
link.next->pheromone = (1.f - pheromone_evaporation) * link.next->pheromone + pheromone_evaporation / total_filament_cost;
Copy link
Owner

Choose a reason for hiding this comment

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

can you revert the changes here?

@SoftFever
Copy link
Owner

@cochcoder
Thank you for your patience.

I'm not very sure about how useful it is, especially since the electricity cost largely depends on the type of filament used. The electricity consumption varies significantly between printing ABS and printing PLA. It's probably more practical to consider the cost within the filament expenses instead. It seems a bit like over-engineering to further break down the costs and clutter the UI.

Hence, I'm inclined not to include the changes.

@FillPavS
Copy link

@cochcoder Thank you for your patience.

I'm not very sure about how useful it is, especially since the electricity cost largely depends on the type of filament used. The electricity consumption varies significantly between printing ABS and printing PLA. It's probably more practical to consider the cost within the filament expenses instead. It seems a bit like over-engineering to further break down the costs and clutter the UI.

Hence, I'm inclined not to include the changes.

Previously, there was a function for calculating the price depending on the amount of plastic used and the operating time of the printer. Now it does not take into account the operating time of the printer. The field where you can specify the price remains in the printer settings, but it does not affect anything.

@cochcoder
Copy link
Contributor Author

@cochcoder

Thank you for your patience.

I'm not very sure about how useful it is, especially since the electricity cost largely depends on the type of filament used. The electricity consumption varies significantly between printing ABS and printing PLA. It's probably more practical to consider the cost within the filament expenses instead. It seems a bit like over-engineering to further break down the costs and clutter the UI.

Hence, I'm inclined not to include the changes.

I see where you're coming from, I'm going to mark this as a draft for now, as I want to look into possibly making a more accurate electricity cost calculator, and see how useful this PR would be in the future.

@cochcoder
Copy link
Contributor Author

@cochcoder Thank you for your patience.

I'm not very sure about how useful it is, especially since the electricity cost largely depends on the type of filament used. The electricity consumption varies significantly between printing ABS and printing PLA. It's probably more practical to consider the cost within the filament expenses instead. It seems a bit like over-engineering to further break down the costs and clutter the UI.

Hence, I'm inclined not to include the changes.

Previously, there was a function for calculating the price depending on the amount of plastic used and the operating time of the printer. Now it does not take into account the operating time of the printer. The field where you can specify the price remains in the printer settings, but it does not affect anything.

I'm confused on what you mean as the cost calculator in place right now does take the time of the print into account.

@FillPavS
Copy link

@cochcoder Thank you for your patience.

I'm not very sure about how useful it is, especially since the electricity cost largely depends on the type of filament used. The electricity consumption varies significantly between printing ABS and printing PLA. It's probably more practical to consider the cost within the filament expenses instead. It seems a bit like over-engineering to further break down the costs and clutter the UI.

Hence, I'm inclined not to include the changes.

Previously, there was a function for calculating the price depending on the amount of plastic used and the operating time of the printer. Now it does not take into account the operating time of the printer. The field where you can specify the price remains in the printer settings, but it does not affect anything.

I'm confused on what you mean as the cost calculator in place right now does take the time of the print into account.

It used to work, but not now. I can specify any printing cost in the printer settings, even 1000000, but this is not added to the price of plastic in the final calculation.

@cochcoder
Copy link
Contributor Author

I'm not having this issue, can you send a screen recording of you replicating this issue?

@FillPavS
Copy link

I'm not having this issue, can you send a screen recording of you replicating this issue?

The built-in screen recording in Windows records only the active window. I had to record it from my phone. I hope it's clear what's going on there)
video - https://cloud.mail.ru/public/sLkn/7gRwY8NRi

@cochcoder cochcoder marked this pull request as draft April 18, 2024 01:51
@cochcoder
Copy link
Contributor Author

@FillPavS So sorry for the late response, I've opened a new PR (#5698) to fix this issue

@g3gg0 g3gg0 mentioned this pull request Jun 19, 2024
Closed
1 task
@SoftFever SoftFever closed this Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SoftFever SoftFever SoftFever left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Advanced costing for print jobs.
3 participants
@cochcoder @SoftFever @FillPavS