-
Notifications
You must be signed in to change notification settings - Fork 0
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
Handle hairline #169
Handle hairline #169
Conversation
Walkthrough
Changes
Assessment against linked issues
Poem
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- translator/vector/symbol/fill.py (3 hunks)
- translator/vector/symbol/line.py (4 hunks)
- translator/vector/symbol/marker.py (2 hunks)
- translator/vector/symbol/utils.py (2 hunks)
Additional comments: 7
translator/vector/symbol/utils.py (2)
- 3-3: このインポート文は適切です。
- 23-33:
get_stroke_width_pt
関数は、ヘアライン幅を適切に扱うための良い実装です。ただし、convert_to_point
関数はこのファイル内で定義されていないため、その実装が正しく動作することを確認してください。translator/vector/symbol/line.py (2)
- 5-5:
to_rgba
とget_stroke_width_pt
のインポートは適切に更新されました。- 15-15:
get_stroke_width_pt
関数の使用は、ヘアライン幅の問題を解決するための適切なアプローチです。この変更により、線シンボルデータの幅が正しく処理されるようになりました。Also applies to: 25-25, 53-53, 61-61, 69-69
translator/vector/symbol/fill.py (1)
- 39-39:
get_stroke_width_pt
関数の使用は、ポリゴンシンボルのアウトライン幅を一貫して処理するための適切な方法です。この変更により、ヘアライン幅の問題が解決されます。Also applies to: 86-86
translator/vector/symbol/marker.py (2)
- 12-12:
get_stroke_width_pt
関数のインポートは適切に行われました。- 177-177:
get_stroke_width_pt
関数の使用は、マーカーシンボルのアウトライン幅を一貫して処理するための適切な方法です。この変更により、ヘアライン幅の問題が解決されます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- translator/vector/symbol/line.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- translator/vector/symbol/line.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but add nits
translator/vector/symbol/utils.py
Outdated
tests/src/python/test_qgssymbollayer_createsld.py#L234 | ||
""" | ||
if defined_width == 0: | ||
return 0.375 # pt (hairline size: 0.5px) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits:
It may be better to take more generic approach, what do you think?
Does it bring more complexity without reasonable benefits?
return 0.375 # pt (hairline size: 0.5px) | |
hairline_width = 0.5 | |
return convert_to_point(hairline_width, PIXEL) # PIXEL should be replaced with correct one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kanahiro
I tried with logic implemented in convert_to_point() function, but it is not showed expected result 0.375pt as expected.
Should I follow QGIS way of conversion from pixel to pt even if is different from common way (1px = 0.75pt) ?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, pixels depends on DPI so it might not fixed value...
Hairline menas the thinest width then it is not bad to use fixed value, it may be okay that it is not 0.375pt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kanahiro Okay method changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- translator/vector/symbol/utils.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- translator/vector/symbol/utils.py
Issue
close #167
変更内容:Description
テスト手順:Test
Summary by CodeRabbit
fill.py
およびline.py
モジュールでconvert_to_point
関数をget_stroke_width_pt
関数に置き換えました。line.py
でto_rgba
関数のインポートをto_rgba, get_stroke_width_pt
に置き換えました。marker.py
でtranslator.vector.symbol.utils
からget_stroke_width_pt
関数を追加しました。get_stroke_width_pt
関数を導入して、ライン幅をポイント単位で計算するようにしました。