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

Refactor issue list #66

Open
6 tasks
andrews05 opened this issue Sep 19, 2022 · 1 comment
Open
6 tasks

Refactor issue list #66

andrews05 opened this issue Sep 19, 2022 · 1 comment

Comments

@andrews05
Copy link
Contributor

andrews05 commented Sep 19, 2022

I've run into a number of issues trying to use the refactor branch - I've made attempts to resolve some of them but the correct solution isn't always clear to me so I figured it best just to open an issue here.

  • block::clear() doesn't seem to work (low priority, pretty sure I don't actually need this)
  • block::set() with a byte count of 1 seems to be setting 4 bytes (is there another way to set bytes?)
  • Fail to read empty resources, caused by incorrect return from block::size() (see Handle empty resources #6 where this occurred previously)
  • PICT encode doesn't write any pixel data (thinks the dimensions are zero?)
  • Assertion failure in packbits::encode() when the writer tries to expand storage (it doesn't own the block)
  • Not an error, but I need to be able to construct a surface from a block in order to encode picts in ResForge
@tjhancocks
Copy link
Collaborator

tjhancocks commented Sep 24, 2022

block::clear() doesn't seem to work (low priority, pretty sure I don't actually need this)

Clear needs to be sorted out, though I'm not certain if its not just an artefact of an older way of doing things.

block::set() with a byte count of 1 seems to be setting 4 bytes (is there another way to set bytes?)

Hmm it's likely that is an issue around the optimisation that is done to speed up memory operations.

auto graphite::data::block::set(uint8_t value, std::size_t bytes, block::position start) -> void
{
    union simd::value v;
    for (unsigned char & byte : v.bytes) {
        byte = value;
    }
    simd::set(get<std::uint32_t *>(start), size() - start, v, bytes);
}

We're expanding out the value into a simd::value which may be either 32-bits or 64-bits depending on the architecture. However, when we provide the pointer to the actual simd::set function, we are doing so as a pointer to an unsigned 32-bit value, so the byte count here is acting more as a number of elements. In the case of none aligned, none word values, we should be bypassing the optimisation here.

Fail to read empty resources, caused by incorrect return from block::size()

I may have fixed this in the last few days? As I was experiencing an issue with the block size. I'll double check this though.

PICT encode doesn't write any pixel data (thinks the dimensions are zero?)

Interesting. Not noticed this one myself, but I'll take a look and see if there are any circumstances that this can occur. That said my usage of the PICT encoder is relatively limited at the moment, so I may have missed a breaking change.

This illustrates why I need to get unit tests in this library sooner rather than later.

Assertion failure in packbits::encode() when the writer tries to expand storage (it doesn't own the block)

This has been a common issue in parts of the library. The behaviour now is to avoid doing deep copies of data if possible. However certain code paths have not been correctly/fully updated to be aware of this.

Not an error, but I need to be able to construct a surface from a block in order to encode picts in ResForge

Cool, I can sort this out. I've considered getting this added anyway.

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

No branches or pull requests

2 participants