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

doc/primer: added introspective example #821

Merged
merged 3 commits into from
Jul 23, 2021
Merged

Conversation

marcus7070
Copy link
Member

@marcus7070 marcus7070 commented Jul 11, 2021

Please read the Read The Docs preview before reading the text diff. The text diff is a bit difficult to read since I used a lot of Sphinx's roles to provide both automatic links to autodocs and formatting. I really like the role formatting since I find it confusing sometimes if text refers to the concept of a workplane or the class Workplane (same for solids, faces, edges, planes, objects/Workplane.objects).

@codecov
Copy link

codecov bot commented Jul 11, 2021

Codecov Report

Merging #821 (7b94514) into master (3cb7237) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 7b94514 differs from pull request most recent head 3c43519. Consider uploading reports for the commit 3c43519 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #821      +/-   ##
==========================================
- Coverage   95.17%   95.15%   -0.02%     
==========================================
  Files          32       32              
  Lines        7436     7489      +53     
  Branches      797      798       +1     
==========================================
+ Hits         7077     7126      +49     
- Misses        221      224       +3     
- Partials      138      139       +1     
Impacted Files Coverage Δ
cadquery/assembly.py 93.13% <100.00%> (+0.34%) ⬆️
cadquery/occ_impl/shapes.py 92.46% <100.00%> (+0.02%) ⬆️
cadquery/occ_impl/solver.py 94.32% <100.00%> (+0.70%) ⬆️
tests/test_assembly.py 100.00% <100.00%> (ø)
tests/test_cadquery.py 99.20% <100.00%> (+<0.01%) ⬆️
cadquery/cqgi.py 79.09% <0.00%> (-2.46%) ⬇️
tests/test_workplanes.py 100.00% <0.00%> (+0.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cb7237...3c43519. Read the comment docs.

@marcus7070 marcus7070 marked this pull request as ready for review July 11, 2021 06:13
@marcus7070
Copy link
Member Author

@Jojain, @fischman, you also were interested in this when I sent it to Google Groups, do you want to have a look here too?

I tried the sys.setprofile method you used @fischman but it wound up being more more code and explanation than this simple print method.

Copy link
Member

@jmwright jmwright left a comment

Choose a reason for hiding this comment

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

This is great @marcus7070 , thanks for writing it. It would be cool if we could somehow give users the ability to use the patched string representations you created to get more information on their chain and modelling context. Maybe even a way to print out the entire stack so they can trace what's happening. I know that CQ-editor already has this covered pretty well, but being able to print the results of each call in the way you have presented could very illuminating for people who are only using the core library.

@marcus7070
Copy link
Member Author

Thanks @jmwright.

It would be cool if we could somehow give users the ability to use the patched string representations you created to get more information on their chain and modelling context.

I'm actually a big fan of just cutting and pasting that bit of code into a user's CQ script. It's reliable and easy, even if it looks a bit ugly.

Maybe even a way to print out the entire stack so they can trace what's happening.

That's an interesting idea. It would be a better fit for the fluent api (rather than stopping at every step to call print, which was easier for an example in the docs but is horrible for a user). Sounds like it would be a good fit for the cadquery-plugins repo.

@fedorkotov
Copy link
Contributor

fedorkotov commented Jul 15, 2021

@marcus7070
Don't know if this should be reported here or as a separate bug. When the model from your example is viewed from a certain angle some lines disappear. I use Firefox 89.0.2 on Windows if that matters
image

doc/primer.rst Outdated
associating the string name given by the :meth:`~cadquery.Workplane.tag` method to the
:class:`~cadquery.Workplane`. Methods such as :meth:`~cadquery.Workplane.workplaneFromTagged` and
selection methods like :meth:`~cadquery.Workplane.edges` can operate on a tagged
:class:`~cadquery.Workplane`. Note that the :meth:`~cadquery.Workplane.tag` method is unusual for a
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand the "Note", Maybe adding an example here would make it more clear

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @marcus7070 from the questions that have been asked in the discord I feel that a lot of them would be resolved by the reading of this part of the docs !

Copy link
Member Author

Choose a reason for hiding this comment

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

3c43519 is aimed at making this more clear, does it help?

@marcus7070
Copy link
Member Author

@fedorkotov

Don't know if this should be reported here or as a separate bug.

It's not limited to this PR, you can recreate the same dissapearing line with this example: https://cadquery.readthedocs.io/en/latest/examples.html#mirroring-from-faces

I think it's just how VTK renders edges; if the two adjacent faces have the same brightness then the edge isn't visible. We don't explicitly ask VTK to render edges, so... not a bug, I guess?

There might be something we can do about it though, VTK has a lot of options.

@adam-urbanczyk
Copy link
Member

Looks good @marcus7070 ! Is it ready for merging?

@marcus7070 marcus7070 merged commit 8fccc0c into master Jul 23, 2021
@marcus7070 marcus7070 deleted the marcus7070/doc-internals branch July 23, 2021 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants