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

Added Layers and Entities true color property #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Olivier-Guy
Copy link

Changes in DwgMergedReader, DwgStreamReaderAC18 and Colors to be able to get RGB true color for layers or entities both with index or true color

Description

Short description about this PR

Tasks done in this PR

  • Something awesome.
  • An evil bug have been defeted.
  • Code cleanup and maintenance has been done.

Related Issues / Pull Requests

  • Add the links of issues or PR related to this one.

Notes for reviewer

  • Things to consider during the review of this PR.

Changes in DwgMergedReader,   DwgStreamReaderAC18 and Colors to be able to get RGB true color for layers or entities both with index or true color
@DomCR DomCR self-requested a review February 19, 2023 08:54
@DomCR
Copy link
Owner

DomCR commented Feb 19, 2023

one test seems to be failing, can you check it please:
[xUnit.net 00:00:07.56] ACadSharp.Tests.ColorTests.HandlesIndexedColors [FAIL]
Skipped ACadSharp.Tests.CadDocumentTests.RemoveLayer [1 ms]
Failed ACadSharp.Tests.ColorTests.HandlesIndexedColors [57 ms]
Error Message:
Assert.True() Failure
Expected: True
Actual: False
Stack Trace:
at ACadSharp.Tests.ColorTests.HandlesIndexedColors() in D:\a\ACadSharp\ACadSharp\ACadSharp.Tests\ColorTests.cs:line 60
[xUnit.net 00:00:08.77] ACadSharp.Tests.IO.DWG.DwgReaderTests.ReadCrcEnabledTest [SKIP]
Skipped ACadSharp.Tests.IO.DWG.DwgReaderTests.ReadCrcEnabledTest [1 ms]

@Olivier-Guy
Copy link
Author

I don't understand why there is an error there, autocad index colors start at 1 and in the test function it start at 0. Maybe this is not a real case.
I am sorry i am starting on github. Maybe i don't know how to do good job.
I was an AutoCAD drafter and now developeper in gis technologies.
Your project is amazing and you made great job, i would like to help your project as well as i can.

@DomCR
Copy link
Owner

DomCR commented Feb 22, 2023

The test also check the method GetTrueColorRbg there is when the test does not pass, you changed the method to return a new Color when is not TrueColor but this method intends to return an array of bytes only if is true color, it returns an empty array if the color is defined by an index.

I hope that I've made some sense, if you have any doubt, need help or want to comment anything about the project development, feel free to open a discussion or an issue to talk about it.

@DomCR
Copy link
Owner

DomCR commented Feb 22, 2023

Another comment about your PR, in the description you may have found that it was already written, that's a template to follow and modify in case that you want to open the PR, is not a big deal that you kept it below your description but is good practice to use it. ;-)

Thanks for your help in the project.

@DomCR DomCR added the enhancement New feature or request label Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants